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

[Merged by Bors] - feat: rewrite the trailing whitespace linter in Lean #16334

Closed
wants to merge 19 commits into from

Conversation

grunweg
Copy link
Collaborator

@grunweg grunweg commented Aug 30, 2024


Open in Gitpod

@grunweg grunweg added the t-linter Linter label Aug 30, 2024
Copy link

github-actions bot commented Aug 30, 2024

PR summary cae8fabf42

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ trailingWhitespaceLinter
- line_endings_check(lines,

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 script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Aug 30, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 31, 2024
@adomani
Copy link
Collaborator

adomani commented Sep 3, 2024

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?

@grunweg
Copy link
Collaborator Author

grunweg commented Sep 3, 2024

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.
Now, batteries could easily gain such infrastructure (I could uplift the text-based linter functionality there, and wait for it to get merged...), but it's less obvious to me how useful that would be. In summary: unless batteries wants to get such functionality, the effort of designing some ad-hoc infrastructure for running the trailing whitespace script and then figuring out how to use that in mathlib seems much higher than just landing the script here, and manually running it on batteries every now and then.

What do you think? Or should one simply try to upstream the text-based linter infrastructure there?

@adomani
Copy link
Collaborator

adomani commented Sep 5, 2024

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.

Mathlib/Tactic/Linter/TextBased.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Linter/TextBased.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Linter/TextBased.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Linter/TextBased.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Linter/TextBased.lean Show resolved Hide resolved
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.
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 5, 2024
@grunweg
Copy link
Collaborator Author

grunweg commented Sep 5, 2024

Thank you for the careful review, Damiano! I responded to all comments, and am interested in your opinion about my opinion :-)

@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Sep 6, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 6, 2024
@grunweg grunweg removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 6, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 12, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 18, 2024
@grunweg
Copy link
Collaborator Author

grunweg commented Oct 8, 2024

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!

@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Oct 31, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Nov 21, 2024
@kim-em
Copy link
Contributor

kim-em commented Nov 22, 2024

bors merge

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the ready-to-merge This PR has been sent to bors. label Nov 22, 2024
mathlib-bors bot pushed a commit that referenced this pull request Nov 22, 2024
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Nov 22, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat: rewrite the trailing whitespace linter in Lean [Merged by Bors] - feat: rewrite the trailing whitespace linter in Lean Nov 22, 2024
@mathlib-bors mathlib-bors bot closed this Nov 22, 2024
@mathlib-bors mathlib-bors bot deleted the MR-trailingWhitespaceLinter branch November 22, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors. t-linter Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants