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

feat: Migrate #print prefix from Mathlib #321

Merged
merged 10 commits into from
Oct 27, 2023
Merged

feat: Migrate #print prefix from Mathlib #321

merged 10 commits into from
Oct 27, 2023

Conversation

joehendrix
Copy link
Contributor

@joehendrix joehendrix commented Oct 25, 2023

This migrates #print prefix while doing some migration of functionality naming to std conventions.

This also restricts the long line check to Std. I had some long lines in the test case file that aren't easy to fix.

Comment on lines 47 to 48
end Elab.Command
end Lean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mario once told me he prefers not to close namespaces if it is only being done at the end of the file. I forget why, however.

I would actually prefer the opposite convention, as it makes it easier doing single-file-minimization (i.e. you can copy a file into a section of another, and only have to move the imports, not adjust namespaces too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a preference personally. You make a good point, but I deleted them for consistency with similar files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe these coding conventions should be recorded somewhere. I also did not know about that one!

Copy link
Member

@digama0 digama0 Oct 26, 2023

Choose a reason for hiding this comment

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

The rule is that you shouldn't close the "main namespace" of the file, to make it easier to end-extend the file. I can also see the point regarding closing the main namespace, but you would have to also put everything in a section in order to really ensure that joining files "just works", because there could also be variables or options outside the namespace (if there even is a namespace).

Copy link
Collaborator

@semorrison semorrison left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

@semorrison semorrison added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Oct 26, 2023
joehendrix and others added 3 commits October 25, 2023 22:47
Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Copy link
Collaborator

@fgdorais fgdorais left a comment

Choose a reason for hiding this comment

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

This is better than my personal version, so I'm approving but I do have a question in the review comments.

/--
Command for #print prefix
-/
syntax (name := printPrefix) "#print prefix " ident : command
Copy link
Collaborator

Choose a reason for hiding this comment

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

My personal version of used "#print" &"prefix" rather than "#print prefix". I'm not sure my version is better but I am interested in learning more about the difference. (It's been so long I don't remember why I did that!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this dates back at least to when it was checked into Mathlib by @digama0

Copy link
Member

@digama0 digama0 Oct 26, 2023

Choose a reason for hiding this comment

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

"#print " "prefix " is better if the intent is to allow more or less whitespace between the tokens. One probably needs to also add a colGt or something, or else unfinished #print statements cannot be differentiated from #print prefix where the prefix just happens to be the next command. The current version "#print prefix " requires exactly one space between #print and prefix, which makes it quite unambiguous but may be seen as undesirable.

The & before "prefix" means that it won't make prefix a token (i.e. not legal as an identifier in e.g. def prefix := 1), but this doesn't make a difference because prefix is already a token (it's a command keyword used for prefix notations). It will parse the next token as an identifier and this may clash with regular #print ident though, so using the token form seems better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I needed to learn this. It seems like I should allow whitespace, but I noticed that i also need withPosition

Consider these two examples:

#print
  prefix
  Nat

#print
prefix
Nat

I think I want to parse the first, but not the second. However the line below parses both:

syntax (name := printPrefix) "#print " (colGt "prefix ") (colGt ident) : command

This works though:

syntax (name := printPrefix) withPosition("#print " (colGt "prefix ") (colGt ident)) : command

Any feedback? I was surprised I needed an explicit withPosition since other similar examples didn't seem to use it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you will need a withPosition because commands are parsed at the top level and this doesn't have a position set. (Tactics, by contrast, appear somewhere after a by and this sets the position.)

@joehendrix joehendrix merged commit 6747f41 into main Oct 27, 2023
2 checks passed
@joehendrix joehendrix deleted the print-prefix branch October 27, 2023 14:12
@joehendrix joehendrix removed the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Oct 28, 2023
bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Oct 29, 2023
This bumps std up to and including the merge of leanprover-community/batteries#321



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
grunweg pushed a commit to leanprover-community/mathlib4 that referenced this pull request Nov 1, 2023
This bumps std up to and including the merge of leanprover-community/batteries#321



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
fgdorais pushed a commit to leanprover-community/mathlib4 that referenced this pull request Nov 1, 2023
This bumps std up to and including the merge of leanprover-community/batteries#321



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
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.

None yet

4 participants