-
Notifications
You must be signed in to change notification settings - Fork 356
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
[Merged by Bors] - feat: rewrite the trailing whitespace linter in Lean #16334
Conversation
grunweg
commented
Aug 30, 2024
•
edited
Loading
edited
PR summary cae8fabf42Import changes for modified filesNo significant changes to the import graph Import changes for all files
Declarations diff
You can run this locally as follows## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>
## more verbose report:
./scripts/declarations_diff.sh long <optional_commit> The doc-module for No changes to technical debt.You can run this locally as
|
I have not yet looked at the PR, but batteries#920 also implements a trailing whitespace linter (actually in two different ways). Is there a way of picking one of these three implementations, get it merged somewhere and possibly upstream one to Batteries? |
I know about the linter in batteries! I am not sure if this will ever get merged... so my thinking is to start with a linter here, and refine later if the batteries script gets merged. In terms of functionality, both do very similar things: the main difference is that this script is integrated with the text-based linters. Batteries has no such infrastructure currently, so needs to invent their own. Mathlib already has something which fits well. What do you think? Or should one simply try to upstream the text-based linter infrastructure there? |
Before upstreaming the whole text-based setup, it should probably be discussed with the maintainers: this may be a much bigger undertaking and, even if accepted, may take a long time. I did not have a chance to look at the code, but I would probably suggest to proceed with this one in its current form. I'll try to review it later today. |
There is an in-progress PR batteries to the same effect: when that PR lands, this linter can be removed. Both implement basically the same logic. This linter is consciously not rewritten as a syntax linter, as a fair number of mathlib contributors have their text editor configured to remove trailing whitespace when saving: with such configurations, in effect, the linter would only fire intermittently (such as, between keypresses), which is not helpful. Running this as a text-based linter in CI, which is auto-fixable, is far more helpful.
2152a3a
to
d11b81c
Compare
bceff34
to
34b6ef3
Compare
Thank you for the careful review, Damiano! I responded to all comments, and am interested in your opinion about my opinion :-) |
I just re-read the discussion: while your suggestions work, I'm not convinced they are better (hence am inclined to leave the code as-is). Let me know if you disagree! |
bors merge |
Co-authored-by: Michael Rothgang <[email protected]>
Pull request successfully merged into master. Build succeeded: |