Skip to content
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

More consistently print requested file name in errors #1274

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

WardBrian
Copy link
Member

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here:
    • OR, no user-facing changes were made

Release notes

Improved --filename-in-msg for the command line version of stanc. Errors now use the requested name, not just warnings.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@WardBrian WardBrian changed the title Fix/printed filename More consistently print requested file name in errors Dec 9, 2022
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #1274 (694f3a2) into master (e7d5dc3) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1274      +/-   ##
==========================================
+ Coverage   88.47%   88.49%   +0.01%     
==========================================
  Files          63       63              
  Lines        9262     9262              
==========================================
+ Hits         8195     8196       +1     
+ Misses       1067     1066       -1     
Impacted Files Coverage Δ
src/middle/Location.ml 94.87% <100.00%> (+1.20%) ⬆️
src/stanc/stanc.ml 84.16% <100.00%> (+0.13%) ⬆️

@WardBrian WardBrian requested a review from nhuurre December 12, 2022 15:55
Copy link
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

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

Great! And stancjs already handled this.

Not important for now but I noticed the docstring for Location.to_string says

If printed_filename is passed, it will replace the filename printed for this Location.t and all recursively included ones.

and I think that the recursive part is a bad idea; overriding the names of #included files has potential for some confusing messages.

@WardBrian
Copy link
Member Author

@nhuurre good catch - that behavior also isn't tested! I've added a test and changed it so that the argument only affects the top-level name (e.g. the file passed on the command line), rather than all of them.

For context, I'm particularly interested in this for things like ivan-bocharov/stan-vscode#11, which sometimes needs to run the compiler on a temporary file. It's nice to print the original name in the error if so.

@WardBrian WardBrian requested a review from nhuurre December 12, 2022 17:23
@nhuurre
Copy link
Collaborator

nhuurre commented Dec 12, 2022

Thanks, but the docstring needs to change too.

@WardBrian WardBrian merged commit 2044426 into master Dec 12, 2022
@WardBrian WardBrian deleted the fix/printed-filename branch December 12, 2022 17:47
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.

2 participants