-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Produce a meaningful error message for invalid module name #1354
Conversation
While the AST form of the error generated by `modules/modulepaths` is handled, the symbol form was not, creating terrible error messages like: Error: invalid module name '<nil tree>' This commit makes cli_reporter aware of the symbol form generated by `modules/modules`, fixing the bad error message.
This allow us to test the error message with testament, as well as giving the user a better idea as to which file is the culprit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you for reviving the fix!
I think the current description is okay as is, but for future PRs, especially those with a more outside-facing change (like this fix), could you focus the summary more on the user-facing changes? Along the lines of "this was the visible previous behaviour, this is how it behaves now", with the implementation details going into the "Details" section.
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
/merge |
Merge requested by: @zerbina Contents after the first section break of the PR description has been removed and preserved below:
|
Summary
While the AST form of the error generated by
modules/modulepaths
ishandled in
cli_reporter
, the symbol form was not, creating terribleerror messages such as:
This PR makes cli_reporter aware of the symbol form generated by
modules/modules
, fixing the bad error message.Details
cli_reporter
now handles the symbol form ofrsemInvalidModuleName
correctly.modules/modules
now raisesrsemInvalidModuleName
with lineinformation, allowing user and tools to know which file the error
originated from.
Fixes #720