-
Notifications
You must be signed in to change notification settings - Fork 546
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
chore: snake-case attributes #1752
Conversation
@@ -360,7 +360,7 @@ def elabMutual : CommandElab := fun stx => do | |||
if isAttribute (← getEnv) attrName then | |||
toErase := toErase.push attrName | |||
else | |||
logErrorAt attrKindStx "unknown attribute [{attrName}]" | |||
logErrorAt attrKindStx m!"unknown attribute [{attrName}]" |
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.
This is a minor bug fix that was revealed by the refactor.
@[builtin_attr_parser] def recursor := leading_parser nonReservedSymbol "recursor " >> numLit | ||
@[builtin_attr_parser] def «class» := leading_parser "class" | ||
@[builtin_attr_parser] def «instance» := leading_parser "instance" >> optional priorityParser | ||
@[builtin_attr_parser] def default_instance := leading_parser nonReservedSymbol "default_instance " >> optional priorityParser |
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 would rather not snake case the definition, but this is required due to a peculiarity in the way attributes are looked up - the parser kind has its namespaces stripped and this has to match the name of the attribute as entered into the attribute map. Refactoring this is probably best left for a future PR though.
builtin_initialize registerBuiltinParserAttribute `builtinDoElemParser ``Category.doElem | ||
builtin_initialize registerBuiltinDynamicParserAttribute `doElemParser `doElem | ||
builtin_initialize registerBuiltinParserAttribute `builtin_doElem_parser ``Category.doElem | ||
builtin_initialize registerBuiltinDynamicParserAttribute `doElem_parser `doElem |
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.
The reason this attribute is not completely snake cased (although I suppose it looks fine as is) is because elab_rules
and macro_rules
make the assumption that you can just stick Parser
(now _parser
) at the end of the category name to derive the parser attribute name. So this function is somewhat misleading and should probably not have the first argument at all, to avoid the implication that it can be changed.
The nix build is failing due to breakage in |
The LeanInk revision is specified in the I must warn you that I have put a small hack in the lockfile¹: the lockfile refers to my personal fork, while the ¹ Nix makes that very easy with |
@gebner I don't suppose I can ask you to do that? I don't have or use nix at all, and while I was hoping to find/replace your commit in the lockfile for my commit (leanprover/LeanInk#37), I can find neither your commit nor indeed |
Oh, I think I force-pushed a fix to that branch before merging the PR but forgot about updating the lockfile. Now I'm kinda surprised that it still builds.. Maybe it's stored in cachix? I've pushed a |
@gebner Any pending issues? If not, let's merge before the PR rots. |
👍 for merging this PR before it rots. We should merge the accumulated LeanInk PRs though (I'd do it myself if I could). |
As discussed in this week's dev meeting, this renames all attributes to use the snake-case convention. More specifically and for future reference, the following attributes have been renamed:
The lake attributes have also been renamed, for consistency: