Repository navigation
[Merged by Bors] - chore(Tactic/DeprecateTo): rename deprecate to into #deprecate to - #43878
SnirBroshi wants to merge 6 commits into
Conversation
PR summary 2c7041c86cImport changes for modified filesNo significant changes to the import graph Import changes for all files
|
|
This looks good to me, thanks! I'm not sure exactly why, but the underscore looks better to me. I feel like lone tokens ought to connect functionality-tokens to commands or to optional arguments, but not to standard arguments, I suppose? Is there a chance we'd either (1) want to put something in front of I think I prefer this without |
|
Just a drive-by comment to say that if this lands, there is a reference to "deprecate to" in the style guide that should be updated as well. |
|
|
Ah yeah, I definitely agree that I think either is fine though, so I'll defer to you + anyone else with an opinion :) I should also mention: this PR should deprecate the |
|
Ah, to be clear, I don't want to remove the |
|
Yeah but why not remove the |
|
I think the |
|
I agree, but as I said above I think that less typing for a very ephemeral command is probably worth it over the nice "to" syntax. |
| -/ | ||
| #guard_msgs in | ||
| deprecate to new_name_mul new_name_add "YYYY-MM-DD" | ||
| #deprecate new_name_mul new_name_add "YYYY-MM-DD" |
There was a problem hiding this comment.
(I'll start a thread here to keep stuff together, but most of the comments on this PR should be seen as part of this thread here)
I think we should go for #deprecate_to xy.
- I quite agree with Snir's first message about how the
_tomakes the syntax clearer. I'd expect#deprecate xyto create analias xy := ...but#deprecate_to xyis fine for the actual behaviouralias ... := xy #deprecate_to xy indoes logically make sense, but then I'd expect the version withoutinto also do something (i.e. act on everything after its declaration). Since this wouldn't really make sense, I think we don't need anin. If you wanted, you could add theinas an optional syntax piece with no effect, just for ergonomics.
There was a problem hiding this comment.
(that being said, its kinda bikeshedding and I'm also fine with #deprecate or #deprecate to, the added # is already a big improvement in consistency)
There was a problem hiding this comment.
And just to be clear, you prefer #deprecate_to to #deprecate to? :) (Personally I like the space instead of the underscore, but I could see it going either way.) EDIT: maybe I loaded the page before the edit of the above comment that mentioned #deprecate to... 🙃
There was a problem hiding this comment.
but then I'd expect the version without
into also do something
What about #whats_new in? Should we remove the in from it as well?
There was a problem hiding this comment.
I mentioned this on another PR recently (where we're debating #find_home for <cmd>), but I think there are two valid uses of <foo> in <cmd>: (1) to say that the effects of foo should be present while elaborating cmd (and not afterwards) (2) to act as a natural-language preposition. I think #whats_new in is really only (2).
I think prepositions as "connective keywords" are okay in general (and may be omitted when appropriate too, e.g. the way we don't need a connective before <cmd> in #deprecate to ... <cmd>), but that's just my preference :)
There was a problem hiding this comment.
that sounds fair 👍
What about #whats_new in? Should we remove the in from it as well?
You do have a good point here. I wouldn't be opposed if #whats_new already parsed by itself, regardless of the result of that was (same as with "in"; something else; just a warning that in is missing). I guess rn you get the parsing error unexpected token '/--'; expected 'in' which is at least instructive.
[...] but I think there are two valid uses of in : (1) to say that the effects of foo should be present while elaborating cmd (and not afterwards) (2) to act as a natural-language preposition.
Tbf, so far I've almost exclusively considered (1)
There was a problem hiding this comment.
I've had another idea about the question regarding #whats_new vs #whats_new in, so I've opened an RFC as #43975 if you want to explore that idea further :) It's not really relevant to this PR, but thought I mention it here
There was a problem hiding this comment.
Great! #deprecate to it is.
FWIW #deprecateto also parses correctly and I don't know how to prevent it (tried some combinations of spaces and ppSpace to no avail), but perhaps we could call it a feature
There was a problem hiding this comment.
I want to argue for #deprecate_to with the underscore, for consistency with the convert_to tactic!
There was a problem hiding this comment.
FWIW
#deprecatetoalso parses correctly and I don't know how to prevent it
I think this is a Lean feature and it's not worth working around it.
Lean has it's very unique approach on how and when whitespace is relevant for parsing...
deprecate to into #deprecate_todeprecate to into #deprecate to
|
There is still some unresolved discussion about maintainer merge The PR keeps the existing pattern (a space in Thank you for the PR! |
|
🚀 Pull request has been placed on the maintainer queue by joneugster. |
|
Thanks! |
|
Pull request successfully merged into master. Build succeeded:
|
deprecate to into #deprecate todeprecate to into #deprecate to
To conform to leanprover-community/leanprover-community.github.io#914
The underscore could also be a space; what do we prefer?And should we add an
insuffix?