[Merged by Bors] - chore(Tactic/GRewrite): rewrite grw family tactic docstrings#36283
[Merged by Bors] - chore(Tactic/GRewrite): rewrite grw family tactic docstrings#36283Vierkantor wants to merge 7 commits intoleanprover-community:masterfrom
grw family tactic docstrings#36283Conversation
This PR rewrites the docstrings for tactics in the `grw` family: `grw`, `grewrite`, `apply_rw`, `apply_rewrite`, `nth_rw` and `nth_rewrite`, to consistently match the official style guide, to make sure they are complete while not getting too long. The `grw` tactic docstring was nice and complete, but the others were very brief and needed expansion (especially since it took me quite some time to understand the motivation behind `apply_rw`).
PR summary 062adc5c07Import changes for modified filesNo significant changes to the import graph Import changes for all files
Declarations diffNo declarations were harmed in the making of this PR! 🐙 You can run this locally as follows## summary with just the declaration names:
./scripts/pr_summary/declarations_diff.sh <optional_commit>
## more verbose report:
./scripts/pr_summary/declarations_diff.sh long <optional_commit>The doc-module for No changes to technical debt.You can run this locally as
|
JovanGerb
left a comment
There was a problem hiding this comment.
Thanks for uniformizing the doc-strings of these tactics! There are a few issues though.
| If an expression `e` is a defined constant, then the equational theorems associated with `e` are | ||
| used. This provides a convenient way to unfold `e`. If `e` has parameters, the tactic will try to | ||
| fill these in by unification with the matching part of the target. Parameters are only filled in | ||
| once per rule, restricting which later rewrites can be found. Parameters that are not filled in | ||
| after unification will create side goals. |
There was a problem hiding this comment.
I don't think we should be explaining how rw works in the doc-string of grw. I'm not even sure myself if this paragraph is correct.
There was a problem hiding this comment.
As far as I can tell they use the same rewrite rule elaborator. And I think it's better to be precise here, unless there's something explicit we can refer to (e.g. a section in the Lean reference manual).
There was a problem hiding this comment.
I would rather not spend many words in the grw docstring talking about what rw does. I think this should be at most one sentence.
The reason is that generalized rewriting is quite a hard concept for many people to understand, and the main goal of the docstring of grw should be to help people understand it.
There was a problem hiding this comment.
I agree that the point of a docstring is to help people understand what a tactic does. But more specifically, the reader should be able to parse and understand a tactic invocation by hovering and getting a docstring. So it is important to be complete and self-contained: we need to mention what happens if there are underscores or e isn't a lemma involving relations.
If this were documented well in the Lean reference manual then we could write something like:
Constants and holes in the expression `e` are treated like `rw [e]` would; see ... for details.But currently this is not mentioned at all, not even in the rw docstring (I have a PR to core Lean that should fix this, but it hasn't been touched in a while).
| If an expression `e` is a defined constant, then the equational theorems associated with `e` are | ||
| used. This provides a convenient way to unfold `e`. |
There was a problem hiding this comment.
Again, this part should not be here.
There was a problem hiding this comment.
I argue for keeping this for the same reason as we should keep it for grw.
|
Many people find it unintuitive that they have to add the The difference with |
joneugster
left a comment
There was a problem hiding this comment.
Looks like the latest review comments haven't been adressed yet, so I'm marking this awaiting-author again
I have rewritten the first paragraphs of the docstrings, hopefully they are clearer now. (I would expect it's less expected that a symmetric relation like equality is assigned a direction. But I trust your experience!) |
|
Thanks, it looks good to me now. At the end it's slightly awkward how |
|
Looks like just one more small comment, then it's good to go! Thanks! bors d+ |
|
✌️ Vierkantor can now approve this pull request. To approve and merge a pull request, simply reply with |
|
Oh right, the build succeeded so: bors r+ |
This PR rewrites the docstrings for tactics in the `grw` family: `grw`, `grewrite`, `apply_rw`, `apply_rewrite`, `nth_rw` and `nth_rewrite`, to consistently match the official style guide, to make sure they are complete while not getting too long. The `grw` tactic docstring was nice and complete, but the others were very brief and needed expansion (especially since it took me quite some time to understand the motivation behind `apply_rw`). Co-authored-by: Anne C.A. Baanen <vierkantor@vierkantor.com>
|
Pull request successfully merged into master. Build succeeded: |
grw family tactic docstringsgrw family tactic docstrings
This PR rewrites the docstrings for tactics in the
grwfamily:grw,grewrite,apply_rw,apply_rewrite,nth_rwandnth_rewrite, to consistently match the official style guide, to make sure they are complete while not getting too long.The
grwtactic docstring was nice and complete, but the others were very brief and needed expansion (especially since it took me quite some time to understand the motivation behindapply_rw).