Skip to content

Modular canonicalizer#1058

Merged
WardBrian merged 9 commits intostan-dev:masterfrom
WardBrian:modular-canonicalizer
Nov 30, 2021
Merged

Modular canonicalizer#1058
WardBrian merged 9 commits intostan-dev:masterfrom
WardBrian:modular-canonicalizer

Conversation

@WardBrian
Copy link
Copy Markdown
Member

@WardBrian WardBrian commented Nov 24, 2021

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 are deprecations, braces, and parenthesis. 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)

Comment on lines +202 to +207
let repair_syntax program settings =
if settings.deprecations then
program
|> map_program
(repair_syntax_stmt (userdef_distributions program.functionblock))
else program
Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Member Author

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.

@WardBrian
Copy link
Copy Markdown
Member Author

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"?

@nhuurre
Copy link
Copy Markdown
Collaborator

nhuurre commented Nov 29, 2021

The changes to the canonicalizer look good. I'm not familiar with cram tests so maybe someone else should review those.

@rok-cesnovar
Copy link
Copy Markdown
Member

I think the js interface can only pass string flags, correct? So each part would need a seperate flag like "canonicalize-deprecations"?

It also handles "name=val" https://github.com/stan-dev/stanc3/blob/master/src/stancjs/stancjs.ml#L114

@WardBrian
Copy link
Copy Markdown
Member Author

Cram tests are described a little in the dune manual. They basically show a command (the line starting with $), its output, and the exit code (in [``]). They're great for testing command line arguments, which is all they're doing here

@WardBrian WardBrian requested a review from nhuurre November 29, 2021 17:01
@WardBrian WardBrian merged commit 45fad64 into stan-dev:master Nov 30, 2021
@WardBrian WardBrian deleted the modular-canonicalizer branch November 30, 2021 18:57
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.

Make Canonicalizer modular

3 participants