Upgrade ruff 0.5.5 -> 0.10.11 & review rules#2486
Merged
Merged
Conversation
yaugenst-flex
force-pushed
the
yaugenst-flex/ruff-upgrade
branch
from
aa0b838 to
500d7d3
Compare
yaugenst-flex
requested review from
daquinteroflex,
frederikschubertflex and
momchil-flex
frederikschubertflex
approved these changes
May 19, 2025
Collaborator
Currently, |
Collaborator
Author
|
Thanks @weiliangjin2021 for clarifying. I think my main source of confusion was that this behavior doesn't seem to be documented, at least I wasn't able to infer it from the field descriptions. The defaults for the fields can be solved by using default factories, which I am changing across the board in #2433. For the classmethods it seems a bit more tricky. One option would be to change the field to @classmethod
def from_layer_bounds(cls, ..., corner_finder: Union[CornerFinderSpec, None, object] = Undefined):
if corner_finder is Undefined:
corner_finder = ... # handle default |
momchil-flex
approved these changes
May 20, 2025
Collaborator
momchil-flex
left a comment
momchil-flex
left a comment
There was a problem hiding this comment.
Sounds good to me. Note that the backend uses the same ruff definitions like the frontend so will need an update too...
- Now enforced:
C901,C408,B904,B028,UP006,UP038,UP035
Seems like C901 is still ignored?
Collaborator
Author
Oops you're right. I disabled Edited the PR description. |
Collaborator
Actually it's in the description: tidy3d/tidy3d/components/grid/grid_spec.py Lines 1023 to 1035 in 3a9ec06
How do we deal with back-compatibility, as right now |
Collaborator
Author
Yes that's fair, in that case it's probably be to go with the sentinel approach in the classmethods, that should be fully backwards-compatible. |
yaugenst-flex
force-pushed
the
yaugenst-flex/ruff-upgrade
branch
from
7cca025 to
a515d75
Compare
Collaborator
Author
|
@weiliangjin2021 implemented the change in 64f7fbf. tests are passing, so just fyi |
daquinteroflex
approved these changes
May 21, 2025
Collaborator
daquinteroflex
left a comment
daquinteroflex
left a comment
There was a problem hiding this comment.
Nice, gone through each rule change and do feel things will be clearer and more standardised by enfocing these. Thanks Yannick.
yaugenst-flex
force-pushed
the
yaugenst-flex/ruff-upgrade
branch
5 times, most recently
from
4a3c50e to
2b85bdb
Compare
Contributor
|
Code Coverage SummaryDetailsDiff against developResults for commit: d114688 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Contributor
|
Changed Files CoverageResults for commit: d114688 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
yaugenst-flex
force-pushed
the
yaugenst-flex/ruff-upgrade
branch
3 times, most recently
from
4a62d80 to
6d47efb
Compare
Run `ruff check . --fix` Run `ruff format .` Move test-specifc `ruff.toml` to `per-file-ignores` Sort test imports Import `Literal` from `typing` Force `from __future__ import annotations` import Rewrite `dict()` as literal (2x faster) Disallow function calls in default arguments Be intentional about exception context Upgrade all (compatible) generics to built-in types Sort `__all__` blocks Enable `RUF` ruleset Prefer tuple unpacking over concatenation Forbid implicit optional No explicit string concatenation Remove unnecessary `pass` statements Remove unnecessary parantheses on raised exceptions Disallow relative imports from parent modules Remove unused import aliases Add lint rule comments Revert "Disallow function calls in default arguments" This reverts commit 12a0a6a. Fix some stragglers Updated ruff version in workflow Lint `data/` dirs rebase fixes sentinel pattern for function calls in default arguments rebase fixes
yaugenst-flex
force-pushed
the
yaugenst-flex/ruff-upgrade
branch
from
6d47efb to
d114688
Compare
Collaborator
|
RIP |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The most controversial rule change in here is probably the addition of
C408, which enforces{"a": a, "b": b}overdict(a=a, b=b).Following reasons why I think it's a good change:
dict()is 2x slower than a literal because it needs to do a global namespace lookup.dict()only works if keys are valid identifiers.I kept all commits in here grouped by the rule and the code changes associated with it, so it's easy to inspect what a specific rule does and if we might want to revert it. I'll keep this PR open and merge before #2433 and resolve any conflicts that arise there.
There are many lines of changes but really only the rule additions need reviewing and whether you agree with them or not.
NOTE: I really want to enable
B008because function calls in default arguments are a significant anti-pattern and they are so common in our codebase that they do affect import time significantly. However, I had to revert the addition because my fix broketests/test_components/test_layerrefinement.py::test_grid_spec_with_layersbecause it seems like many of thegrid_specfunctions have undocumented behavior when it comes to passingNoneforcorner_finderandcorner_refinement. If you could have a look @weiliangjin2021 that would be great.PR Summary
Net Changes
C4*,NPY*,RUF*,ISC*,PIE*,RSE*,TID*,PLE*,PLC*C*, standaloneNPY201RUF001-003,RUF012,RUF015,NPY002C408,B904,B028,UP006,UP038,UP035[tool.ruff]section inpyproject.toml:select->extend-selectto keep default rules, similarly forignoretests/ruff.tomlto[tool.ruff.lint.per-file-ignores]inpyproject.tomlAdditions
C4,NPY,RUF,ISC,PIE,RSE,TID,PLE,PLCinextend-selectrequired-imports = ["from __future__ import annotations"]UP006.RUF001-003,RUF012,RUF015,NPY002viaextend-ignoretests/**/*(B015,E402,TID252, …)Removed / No-Longer-Ignored
typing-moduleshackLiteraltypes; custom list deleted.ignoreitems:C408,B904,B028,UP006,UP038,UP035UP006safe thanks to required imports.Cprefix, loneNPY201C4*and fullNPY*suites.