-
-
Notifications
You must be signed in to change notification settings - Fork 685
Fix and enable ruff UP006 #41108
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
base: develop
Are you sure you want to change the base?
Fix and enable ruff UP006 #41108
Conversation
| # 2 PLE0302 [ ] The special method `__len__` expects 1 parameter, 3 were given | ||
| # 1 F402 [ ] Import `factor` from line 259 shadowed by loop variable | ||
| # 1 PLC0208 [*] Use a sequence type instead of a `set` when iterating over values | ||
| # 10589 PLC0415 [ ] import-outside-top-level |
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 updated this comment while I was touching this file. Looks like that hasn't been done in a while.
| [tool.ruff.lint] | ||
| ignore = [ | ||
| "E501", # Line too long - hard to avoid in doctests, and better handled by black. | ||
| "UP045", # non-pep604-annotation-optional - Whether `Optional[T]` or `T | None` is better is subjective. |
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.
In preparation for eventually being able to enable more/all UP rules. At first glance this was the only one I saw that seemed potentially controversial. I'm not taking a stance on it for this PR, but my personal opinion is that Optional is more readable for parameters and less readable for return values.
Other rules in UP that would have a big impact for us: changing old-style string formatting to f-strings. I think that's only an issue for us because of how many files it would change. In principle I think f-strings are far more readable, but unless we want to do a patch bomb it's hard to see us ever transitioning the entire code base to f-strings. (I guess in theory we could do some kind of git hook autoformat when files get changed for other reasons to slowly transition things over automatically, but I think that would be controversial.)
|
Documentation preview for this PR (built with commit 787b086; changes) is ready! 🎉 |
Replace things like
List[T]withlist[T]in type annotations. Enables this rule in CI and in configuration files.To do this I used ruff to fix UP006, then fixed F401 on the changed files (to remove the UP006 unused imports left behind by UP006).
There are several other ruff UP rules that have autofix and can easily be enabled. To keep the PR a reasonable size we only do UP006 here.
📝 Checklist