Skip to content

[Merged by Bors] - chore: remove style-exceptions.txt#16417

Closed
grunweg wants to merge 3 commits intomasterfrom
MR-remove-styleexceptions
Closed

[Merged by Bors] - chore: remove style-exceptions.txt#16417
grunweg wants to merge 3 commits intomasterfrom
MR-remove-styleexceptions

Conversation

@grunweg
Copy link
Copy Markdown
Contributor

@grunweg grunweg commented Sep 2, 2024

This file has been recording exceptions to the text-based style linters in lint-style.py and Linter/TextBased. The length of this file is meant to reach zero - and now, it indeed has. 🎉

Some of the linter errors were easy to fix (and just required sustained effort), such as some misformatted author lines or missing module documentation. The main remaining entries were exceptions for the "long file linter", which has been rewritten as a syntax linter (in #15610 and #16299). Exceptions are now tracked as set_option linter.style.longFile <number> instead, obviating the need for this file.

This implies the need for a file with transient style exceptions is gone. All of the remaining text-based linters should not be introduced in new code. For permanent exceptions, the file nolints-style.txt continues to exist; that file must (intentionally) be updated by hand.

zulip discussion


Open in Gitpod

@grunweg grunweg added the t-linter Linter label Sep 2, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 2, 2024

PR summary e0559f468b

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

- OutputSetting

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.

@grunweg grunweg force-pushed the MR-remove-styleexceptions branch from 6f8390d to 8c56300 Compare September 2, 2024 16:01
@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 2, 2024
@grunweg grunweg force-pushed the MR-remove-styleexceptions branch from 92e3842 to 16c188d Compare September 3, 2024 09:56
@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 3, 2024
@bryangingechen
Copy link
Copy Markdown
Contributor

How confident are we that we won't need to add new transient exceptions for new linters in the future? Or is the idea that they will also be tracked with set_option in the files themselves?

The code otherwise looks like it does what it claims to (but admittedly this is my first time looking at the lean4 style linter code).

Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
@grunweg
Copy link
Copy Markdown
Contributor Author

grunweg commented Sep 3, 2024

How confident are we that we won't need to add new transient exceptions for new linters in the future? Or is the idea that they will also be tracked with set_option in the files themselves?

The code otherwise looks like it does what it claims to (but admittedly this is my first time looking at the lean4 style linter code).

That's the right question to ask. Personally, I am willing to make a bet that

  • the linter can be fixed beforehand (indeed, that's the normal procedure for syntax linters), or
  • these linters won't appear, or
  • we can always resurrect the exceptions file. (The logic for parsing the file is still there, to support nolints-style.txt. Just the automated infrastructure is gone, and the need for lint-style.py to look at the exceptions file.)

@joneugster
Copy link
Copy Markdown
Contributor

I agree with @grunweg, currently it is quite common that a new linter lives in a PR until most of the things it marks have been addressed, which is a good habit. And the PR description now provides good reasoning about future exemptions

I think it's safe to remove the file and drop one more technical depth.

maintainer merge

@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 4, 2024

🚀 Pull request has been placed on the maintainer queue by joneugster.

@github-actions github-actions bot added the maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. label Sep 4, 2024
@urkud
Copy link
Copy Markdown
Member

urkud commented Sep 4, 2024

Thanks! 🎉
bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Sep 4, 2024
mathlib-bors bot pushed a commit that referenced this pull request Sep 4, 2024
This file has been recording exceptions to the text-based style linters in `lint-style.py` and `Linter/TextBased`. The length of this file is meant to reach zero - and now, it indeed has. 🎉

Some of the linter errors were easy to fix (and just required sustained effort), such as some misformatted author lines or missing module documentation. The main remaining entries were exceptions for the "long file linter", which has been rewritten as a syntax linter (in #15610 and #16299). Exceptions are now tracked as `set_option linter.style.longFile <number>` instead, obviating the need for this file.

This implies the need for a file with transient style exceptions is gone. All of the remaining text-based linters should not be introduced in new code. For *permanent* exceptions, the file `nolints-style.txt` continues to exist; that file must (intentionally) be updated by hand.

[zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/RFC.3A.20remove.20.60scripts.2Fstyle-exceptions.2Etxt.60)
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Sep 5, 2024

This PR was included in a batch that was canceled, it will be automatically retried

mathlib-bors bot pushed a commit that referenced this pull request Sep 5, 2024
This file has been recording exceptions to the text-based style linters in `lint-style.py` and `Linter/TextBased`. The length of this file is meant to reach zero - and now, it indeed has. 🎉

Some of the linter errors were easy to fix (and just required sustained effort), such as some misformatted author lines or missing module documentation. The main remaining entries were exceptions for the "long file linter", which has been rewritten as a syntax linter (in #15610 and #16299). Exceptions are now tracked as `set_option linter.style.longFile <number>` instead, obviating the need for this file.

This implies the need for a file with transient style exceptions is gone. All of the remaining text-based linters should not be introduced in new code. For *permanent* exceptions, the file `nolints-style.txt` continues to exist; that file must (intentionally) be updated by hand.

[zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/RFC.3A.20remove.20.60scripts.2Fstyle-exceptions.2Etxt.60)
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Sep 5, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: remove style-exceptions.txt [Merged by Bors] - chore: remove style-exceptions.txt Sep 5, 2024
@mathlib-bors mathlib-bors bot closed this Sep 5, 2024
@mathlib-bors mathlib-bors bot deleted the MR-remove-styleexceptions branch September 5, 2024 01:24
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
This file has been recording exceptions to the text-based style linters in `lint-style.py` and `Linter/TextBased`. The length of this file is meant to reach zero - and now, it indeed has. 🎉

Some of the linter errors were easy to fix (and just required sustained effort), such as some misformatted author lines or missing module documentation. The main remaining entries were exceptions for the "long file linter", which has been rewritten as a syntax linter (in #15610 and #16299). Exceptions are now tracked as `set_option linter.style.longFile <number>` instead, obviating the need for this file.

This implies the need for a file with transient style exceptions is gone. All of the remaining text-based linters should not be introduced in new code. For *permanent* exceptions, the file `nolints-style.txt` continues to exist; that file must (intentionally) be updated by hand.

[zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/RFC.3A.20remove.20.60scripts.2Fstyle-exceptions.2Etxt.60)
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
This file has been recording exceptions to the text-based style linters in `lint-style.py` and `Linter/TextBased`. The length of this file is meant to reach zero - and now, it indeed has. 🎉

Some of the linter errors were easy to fix (and just required sustained effort), such as some misformatted author lines or missing module documentation. The main remaining entries were exceptions for the "long file linter", which has been rewritten as a syntax linter (in #15610 and #16299). Exceptions are now tracked as `set_option linter.style.longFile <number>` instead, obviating the need for this file.

This implies the need for a file with transient style exceptions is gone. All of the remaining text-based linters should not be introduced in new code. For *permanent* exceptions, the file `nolints-style.txt` continues to exist; that file must (intentionally) be updated by hand.

[zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/RFC.3A.20remove.20.60scripts.2Fstyle-exceptions.2Etxt.60)
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 12, 2024
This file has been recording exceptions to the text-based style linters in `lint-style.py` and `Linter/TextBased`. The length of this file is meant to reach zero - and now, it indeed has. 🎉

Some of the linter errors were easy to fix (and just required sustained effort), such as some misformatted author lines or missing module documentation. The main remaining entries were exceptions for the "long file linter", which has been rewritten as a syntax linter (in #15610 and #16299). Exceptions are now tracked as `set_option linter.style.longFile <number>` instead, obviating the need for this file.

This implies the need for a file with transient style exceptions is gone. All of the remaining text-based linters should not be introduced in new code. For *permanent* exceptions, the file `nolints-style.txt` continues to exist; that file must (intentionally) be updated by hand.

[zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/RFC.3A.20remove.20.60scripts.2Fstyle-exceptions.2Etxt.60)
mathlib-bors bot pushed a commit that referenced this pull request Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. 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.

5 participants