-
Notifications
You must be signed in to change notification settings - Fork 681
Incrementally improve linting setup #1209
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
Conversation
amjith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of not bothering with exceptions and going with the defaults and let ruff check --fix do the work. I realize we need a minimal set of exceptions for conflicting rules. Other than that we can go all in.
pyproject.toml
Outdated
| 'F541', # f-string without placeholders | ||
| 'PIE808', # range() starting with 0 | ||
| # https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules | ||
| 'E501', # Line too long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think E501 is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it in this PR, but the point is that sometimes ruff check and ruff format have different opinions about the line length. We can always add it back if there is friction.
pyproject.toml
Outdated
| 'E401', # Multiple imports on one line | ||
| 'E402', # Module level import not at top of file | ||
| 'E501', # Line too long | ||
| 'F541', # f-string without placeholders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove F541 as well and fix all the places that violate this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. F541 was an intentional style choice. I think there is nothing wrong with f-strings lacking placeholder.
* let lint target the oldest supported Python version * disable lint rules which may conflict with the formatter, per Astral recommendations, and document * establish rules for sorting imports (but don't activate this yet in CI) * enable preview mode for "ruff format", and reformat code, making some whitespace tighter
e6f92b3 to
02baf65
Compare
We are already going further than "all in" by enabling some non-default lint series. However, we aren't enabling |
Description
Enabling
preview = truefor ruff might be controversial, but I've had only good luck with it.The ignored lint rules wouldn't be needed by default, but the "E" and "W" series are explicitly activated.
Checklist
I've added this contribution to thealready coveredchangelog.md.AUTHORSfile (or it's already there).