-
-
Notifications
You must be signed in to change notification settings - Fork 748
isort → ruff #9152
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
isort → ruff #9152
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 27 files ±0 27 suites ±0 9h 47m 59s ⏱️ - 14m 39s For more details on these failures, see this check. Results for commit e824312. ± Comparison against base commit d8e3fd2. ♻️ This comment has been updated with latest results. |
cd07995 to
4b1f9d1
Compare
| "distributed/utils.py" = ["F821"] | ||
|
|
||
| [tool.isort] | ||
| sections = ["FUTURE", "STDLIB", "THIRDPARTY", "DISTRIBUTED", "FIRSTPARTY", "LOCALFOLDER"] |
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.
It looks like ruff supports sections, can you make sure we are configuring things the same way? We use these sections specifically to improve readability.
I would suggest going through each setting we had with isort and checking if you can replicate the same setting with ruff.
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.
Like for dask, I focused on actual changes in the source code instead of reviewing individual options in detail. It seems to work well in both repositories.
I can review the options more in detail. I seem to recall that most of them are identical to the defaults, which is why I removed them. I'll have a look.
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.
tool.isort.sections defines all sections isort uses by default:
from typing import Tuple
FUTURE: str = "FUTURE"
STDLIB: str = "STDLIB"
THIRDPARTY: str = "THIRDPARTY"
FIRSTPARTY: str = "FIRSTPARTY"
LOCALFOLDER: str = "LOCALFOLDER"
DEFAULT: Tuple[str, ...] = (FUTURE, STDLIB, THIRDPARTY, FIRSTPARTY, LOCALFOLDER)The main difference with the default value is the addition of DISTRIBUTED.
- "FUTURE", "STDLIB", "THIRDPARTY", "DISTRIBUTED", "FIRSTPARTY", "LOCALFOLDER"
+ "FUTURE", "STDLIB", "THIRDPARTY", "FIRSTPARTY", "LOCALFOLDER"where DISTRIBUTED is defined as:
known_distributed = ["dask", "zict"]
which I guess forces to identify puts dask, distributed and zict as being part of the current python project. This avoids the empty line between dask and distributed importsdask and zict imports in a section of their own, between third-party and first-party distributed.
My thoughts about it:
- Why care about such details and not use the defaults?
Actually, why avoid the line betweendaskanddistributedimports? I don't see how that's a good idea.
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.
Nevertheless, I have added this section in ruff settings:
"distributed" = ["dask", "zict"]
I'll have a fresh look at dask taking into account this change.
| profile = "black" | ||
| skip_gitignore = true | ||
| force_to_top = ["true"] | ||
| default_section = "THIRDPARTY" |
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.
Obviously, profile = "black" is not needed any more.
I think that skip_gitignore has no ruff equivalent, and that it's basically a non-issue.
Option force_to_top seems misused here.
4b1f9d1 to
e824312
Compare
jacobtomlinson
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.
This looks good many thanks!
No description provided.