Skip to content
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

Apply some ruff rules #2353

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

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jan 29, 2025

Some optimizations for performance or readability.
% ruff check --select=C4,E713,FLY,PERF401,PIE810,PLC0414,RUF005,RUF010,RUF021,SIM118,SIM201,UP004,UP033,W

15	RUF005 	[*] collection-literal-concatenation
10	RUF010 	[*] explicit-f-string-type-conversion
 7	RUF021 	[*] parenthesize-chained-operators
 4	E713   	[*] not-in-test
 3	PIE810 	[*] multiple-starts-ends-with
 3	PERF401	[ ] manual-list-comprehension
 2	SIM118 	[*] in-dict-keys
 1	C413   	[*] unnecessary-call-around-sorted
 1	C417   	[*] unnecessary-map
 1	SIM201 	[*] negate-equal-op
 1	FLY002 	[*] static-join-to-f-string
 1	PLC0414	[*] useless-import-alias
 1	UP004  	[*] useless-object-inheritance
 1	UP033  	[*] lru-cache-with-maxsize-none
[*] fixable with `ruff check --fix`

https://docs.astral.sh/ruff/rules

I do not understand DeepSource's complaints that two functions have increased cyclomatic complexity when both ruff and flake8 agree that the cyclomatic complexity of these functions is unchanged.

% ruff check --select=C901 # Results are the same on main and this branch.

isort/identify.py:43:5: C901 `imports` is too complex (33 > 10)
isort/settings.py:827:5: C901 `_get_config_data` is too complex (27 > 10)

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.16%. Comparing base (e169a70) to head (a394f3a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2353   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files          40       40           
  Lines        3098     3098           
  Branches      680      680           
=======================================
  Hits         3072     3072           
  Misses         15       15           
  Partials       11       11           

@cclauss cclauss force-pushed the some-ruff-optimizations branch from 72307c0 to 895d54f Compare January 29, 2025 19:48
Copy link
Member

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staticdev What do you think of adding ruff to the linters? I don't really like making changes based on a linters output but then not adding the linter to CI to ensure the changes don't creep back in.

@staticdev
Copy link
Collaborator

@staticdev What do you think of adding ruff to the linters? I don't really like making changes based on a linters output but then not adding the linter to CI to ensure the changes don't creep back in.

I don't see a problem we can add ruff linter.

Copy link
Contributor

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems strange to apply ruff rules to a project that is not configured to follow ruff rules.

On a personal note, I view ruff as harmful to the Python ecosystem, as its paid developers cannibalize cumulative decades of others' work in a language that Python developers may not know how to contribute in. It's particularly galling that their homepage prominently features a quote from Timothy Crosley, who started and effectively abandoned isort itself sometime after ruff copied the project under a config option blatantly named isort.

Anyway, I've left feedback on the PR. ruff introduced what appears to be an equivalently-unnecessary !s in several f-strings that I suggest addressing.

tests/unit/test_main.py Outdated Show resolved Hide resolved
tests/unit/test_main.py Outdated Show resolved Hide resolved
@staticdev
Copy link
Collaborator

staticdev commented Feb 5, 2025

It seems strange to apply ruff rules to a project that is not configured to follow ruff rules.

On a personal note, I view ruff as harmful to the Python ecosystem, as its paid developers cannibalize cumulative decades of others' work in a language that Python developers may not know how to contribute in. It's particularly galling that their homepage prominently features a quote from Timothy Crosley, who started and effectively abandoned isort itself sometime after ruff copied the project under a config option blatantly named isort.

Anyway, I've left feedback on the PR. ruff introduced what appears to be an equivalently-unnecessary !s in several f-strings that I suggest addressing.

@kurtmckee I am contributing also to open-source software since before Github exists, I feel you and share partly your opinion. But I have a long disclaimer, even being maintainer for isort, I am using ruff in many of my projects. I respect the diversity and competition of the projects and want them to thrive with isort, or ruff, or the next cool tool. This for me is kinda the beauty of open-source.

Ruff is definetely fast but also feels immature and incomplete now. Astral for me is doing a good service, but needs some years maybe to catchup with all ambitious projects.

Tldr: i think isort and ruff can coexist in piece. And we should make them help developers have a better life.

Copy link
Contributor

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code change looks good to me.

@cclauss
Copy link
Contributor Author

cclauss commented Feb 5, 2025

I do not have merge rights so a maintainer would need to approve and merge.

Should I add ruff to CI in this PR or in a separate PR?

@kurtmckee
Copy link
Contributor

(By the way, @cclauss and @staticdev, I sometimes see you both pop up in other repos I interact with -- thanks for the work you both do!)

Copy link
Collaborator

@staticdev staticdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cclauss, I think this will improve our code readability. LGTM.
Thanks also @kurtmckee for the review.

@DanielNoord also good for you?

@staticdev
Copy link
Collaborator

staticdev commented Feb 5, 2025

I do not have merge rights so a maintainer would need to approve and merge.

Should I add ruff to CI in this PR or in a separate PR?

@cclauss I think this one already have a nice size. I would advice doing it in another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants