-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Modular canonicalizer #1058
Modular canonicalizer #1058
Conversation
let repair_syntax program settings = | ||
if settings.deprecations then | ||
program | ||
|> map_program | ||
(repair_syntax_stmt (userdef_distributions program.functionblock)) | ||
else program |
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.
I'm inclined to say that repair_syntax
should be enabled for all formatting, not just deprecations. It's not about syntax that's going to be deprecated but syntax that has already been deprecated.. A program that "needs" repair_syntax
isn't even going to typecheck without it.
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.
At the moment, those programs do fail to typecheck without --print-canonical
. If we don't like that we could run it if and only if the formatter is activated, I suppose.
As @rok-cesnovar brought up in #577, it would be useful if this was also available in stancjs. Any suggestions on how that should be implemented? I think the js interface can only pass string flags, correct? So each part would need a seperate flag like "canonicalize-deprecations"? |
The changes to the canonicalizer look good. I'm not familiar with cram tests so maybe someone else should review those. |
It also handles "name=val" https://github.com/stan-dev/stanc3/blob/master/src/stancjs/stancjs.ml#L114 |
Cram tests are described a little in the dune manual. They basically show a command (the line starting with |
Resolves #1053. I think this will be good for #1049 as we can run only the deprecations setting on the test suite so we don't over-regularize.
This also lays the ground work for #1042 but I ran into some issues after fixing a bug in how we handle source positions that I need more time to run down, so I'm submitting this without that work.
Submission Checklist
Release notes
The canonicalizer can now have each part enabled seperately. This can be done with commands like
stanc --auto-format --canonicalize=braces,deprecations <model>
. Current options aredeprecations
,braces
, andparenthesis
. The--print-canonical
flag still works, but is simply synonymous for--auto-format --canonicalize=<all possible options>
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)