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

refactor(lint-style): various small clean-ups #12596

Closed
wants to merge 7 commits into from

Conversation

grunweg
Copy link
Collaborator

@grunweg grunweg commented May 2, 2024

  • alphabetise imports
  • ensure errors are distinct
  • convert the list of style errors to an enum
  • use this to deduplicate and simplify printing errors and parsing exceptions

See individual commit messages for details. Each commit can be reviewed independently.

Open in Gitpod

@grunweg grunweg added awaiting-review CI Modifies the continuous integration / deployment setup labels May 2, 2024
@Ruben-VandeVelde
Copy link
Collaborator

I'm not sure I agree with the first commit: if the script can produce an error, we should be able to except it

@grunweg
Copy link
Collaborator Author

grunweg commented May 5, 2024

I see your point, but I would argue that the particular errors I'm removing should never land in mathlib anyway - so I don't see why excepting them would be useful! (If there's the need to make the build pass, commenting that linter is another option.)

(FWIW, the script currently doesn't implement excepting all errors, so the current logic is already different.)

@grunweg
Copy link
Collaborator Author

grunweg commented May 10, 2024

I pushed a few more commits, refactoring the script further: in particular, parsing style-exceptions now truly recognises all possible errors, in fewer lines of code than before.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label May 11, 2024
@@ -65,20 +68,12 @@
for exline in f:
filename, _, _, _, _, errno, *extra = exline.split()
path = ROOT_DIR / filename
if errno == "ERR_COP":
Copy link
Collaborator Author

@grunweg grunweg May 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes some variants; a later commit implements parsing all exception types (making this obsolete).
(I didn't feel like editing history again to edit this part out.)

@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 May 11, 2024
@grunweg grunweg added the t-linter Linter label May 23, 2024
@grunweg
Copy link
Collaborator Author

grunweg commented Jun 9, 2024

For the record: #13199 will make this PR obsolete. If you'd like to review this, reviewing the PRs split off #13199 might be more useful long-term.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jun 10, 2024
@grunweg
Copy link
Collaborator Author

grunweg commented Jun 30, 2024

Let me close this PR in favour of #13199 (and its pieces).

@grunweg grunweg closed this Jun 30, 2024
@grunweg grunweg deleted the MR-cleanup-lint-style branch June 30, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Modifies the continuous integration / deployment setup merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) t-linter Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants