Skip to content

Misc lints #4694

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Misc lints #4694

wants to merge 13 commits into from

Conversation

MeGaGiGaGon
Copy link
Collaborator

@MeGaGiGaGon MeGaGiGaGon commented Jun 13, 2025

Description

I was bored so I decided to run all the ruff lints against src/black, and only fix the ones I liked, so here's the results. I tried my best to only pick ones that I thought made a clear improvement to the code without causing too much churn. All the reasoning is in the commit messages, but I also copied it here because github cuts it off.

SIM201 negate-equal-op seems reasonable

RUF010 explicit-f-string-type-conversion seems reasonable

RUF003 ambiguous-unicode-character-comment churn but reasonable

RUF001 ambiguous-unicode-character-string changing the literal NBSP to using \N{} is a lot more editor friendly, good change

PLR1736 unnecessary-list-index-lookup good change, both speed and clarity improvement

PLC2401 non-ascii-name churn but good, original name takes a long time to parse that it is the empty set and is very had to type, while set() is more explicit and easy to type
Reverted since there were PRs on this before per Jelle

PERF401 manual-list-comprehension code clarity improvement

FURB136 if-expr-min-max code clarity improvement
Reverted, was on the edge with including this and is maybe worse perf per Jelle

SIM300 yoda-conditions improves readability and consistency

D301 escape-sequence-in-docstring some docstrings were broken due to having unescaped \n s, so I fixed those, as well as converted some others for consistency

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
    • N/A no behavior changes made
  • Add / update tests if necessary?
    • N/A no tests affected
  • Add new / update outdated documentation?
    • N/A no docs affected

@MeGaGiGaGon MeGaGiGaGon added C: cleanup Refactoring and removing dust :) skip news Pull requests that don't need a changelog entry. labels Jun 13, 2025
@tusharsadhwani
Copy link
Collaborator

@MeGaGiGaGon for longevity i think it's better if you could:

  • Instead of selectively fixing the raised lints, create a ruff config that lets anyone else run the lints in a way that this PR passes the lint step

  • Add a CI step that runs ruff so we can maintain the code quality going forward

@MeGaGiGaGon
Copy link
Collaborator Author

The main reason I didn't do that is since especially with lints, I feel like it should be a lot more thought out and discussed than my approach. Since what lints you use defines the style of the code, I feel like any major changes to it, like switching to a whole new linting framework, should be a whole team effort.

I'm also very new to linting in general, and the main reason I use ruff is that it is the thing I found first, followed by it having a lot of lints so it's very effective with my "enable all then trim" strategy. I'm unsure what else is out there in the ecosystem, which goes back to the "this should probably be a bigger discussion".

And even assuming that ruff would be chosen as the linter, I was very conservative with my choices here. There are some lints I saw that I do like, but would cause a large amount of churn to enable for smaller benefits, which is why I only went with ones that are both fairly contained and seem like clear improvements.

For context, there were 90 different lints raised, with 1253 total errors. Out of those I chose 10 lints to fix, making up 16 of the errors.

This also leads into/has ties with a different discussion I feel is worth having, which is on linting blib2to3. Currently it is still excluded from coverage, and also has a lot more lints that could be actually worth fixing. At least for my changes, a lot of them have been inside blib2to3, so I've been wondering if it would be worthwhile to add it to the lints and clean up the code.

That also has ties to a discussion I'm not sure ever got finished, #2318 (Switch to a new parser), and might be worth re-discussing since I'm not sure how the parser landscape has changed since 2021.

Also I'm tired, so sorry for the long wall of text. I'll probably come back and edit this to make it more readable tomorrow morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: cleanup Refactoring and removing dust :) skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants