Skip to content

Convert the code snippets in some standard modules to tests#27491

Merged
dlongnecke-cray merged 4 commits intochapel-lang:mainfrom
dlongnecke-cray:make-docs-testable-4
Aug 7, 2025
Merged

Convert the code snippets in some standard modules to tests#27491
dlongnecke-cray merged 4 commits intochapel-lang:mainfrom
dlongnecke-cray:make-docs-testable-4

Conversation

@dlongnecke-cray
Copy link
Contributor

@dlongnecke-cray dlongnecke-cray commented Jul 7, 2025

Convert the code snippets in a bunch more standard modules to testable examples.

Reviewed by @lydia-duncan. Thanks!

@dlongnecke-cray
Copy link
Contributor Author

Huh, not sure what to make of those failing CI checks.

@DanilaFe
Copy link
Contributor

DanilaFe commented Jul 9, 2025

Huh, not sure what to make of those failing CI checks.

David and I talked about this on slack, likely this is due to testCompilerError locking down exact line numbers. This PR slightly changed the line numbering in Errors.chpl.

ensureErrorOnLine(ctx, guard.errors(), ErrorType::UserDiagnosticEncounterError, 645, "no", /* allowOthers = */ true);

@lydia-duncan lydia-duncan self-requested a review July 17, 2025 20:02
Copy link
Member

@lydia-duncan lydia-duncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing to fix, one question that I have. Thanks for taking this on!


/* START_EXAMPLE_2 */
// (optional) if we counted previously, reset the counters to zero
resetCommDiagnostics();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a resetCommDiagnostics call in between STOP_EXAMPLE_0 and START_EXAMPLE_1? Or is that unnecessary since we did a Here call instead?

@dlongnecke-cray
Copy link
Contributor Author

Thanks for the feedback. I got distracted and sat on this for awhile and let it get stale. Rebasing and addressing your comments now.

@lydia-duncan
Copy link
Member

No worries, there's a lot going on :)

Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
@dlongnecke-cray
Copy link
Contributor Author

Just rebased and pushed fixes in response to your feedback.

@dlongnecke-cray
Copy link
Contributor Author

@lydia-duncan I took the liberty of merging while it's still Thursday, hope you're OK with that.

@dlongnecke-cray dlongnecke-cray merged commit 7778795 into chapel-lang:main Aug 7, 2025
10 checks passed
@lydia-duncan
Copy link
Member

Yeah, that was the right call!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants