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.

Sign up for GitHub

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

Jump to bottom

STY: migrate linting to ruff #4320

Merged
merged 4 commits into from
Feb 15, 2023
Merged

STY: migrate linting to ruff #4320

merged 4 commits into from
Feb 15, 2023

Conversation

neutrinoceros
Copy link
Member

PR Summary

Fix #4203

This brings pre-commit run --all-files down to 5s on my machine (as opposed to 22s on the main branch), and pre-commit.ci runs complete in 30s instead of 60s

ruff is not completely stable yet but many big projects are already using it (pandas, scipy, jupyter, to name a few), and I wanted to see how big of an effort migration would actually be.
The patch I end up with is small enough to my taste that I feel confident to submit it for others to consider.

I discovered that ruff is slightly more agressive than pyupgrade on a couple rules, especially when it comes to replacing old string formatting (%) with more modern equivalents, but even at the scale of our code base adoption appears straightforward !

@neutrinoceros neutrinoceros added infrastructure Related to CI, versioning, websites, organizational issues, etc code style Related to linting tools labels Jan 31, 2023
matthewturk
matthewturk previously approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

I'm still pretty ambivalent about switching right now, even if there are other players doing it..

yt/funcs.py
@@ -1191,14 +1191,13 @@ def validate_object(obj, data_type):

def validate_axis(ds, axis):
if ds is not None:
valid_axis = ds.coordinates.axis_name.keys()
valid_axis = sorted(
list(ds.coordinates.axis_name.keys()), key=lambda k: str(k).swapcase()
Copy link
Member

Choose a reason for hiding this comment

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

What is this doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorting keys. When sorting strings, uppercase comes before lower case, so I'm reverting that order with a custom lambda key to match how the default list ([0, 1, 2, "x", "y", "z", "X", "Y", "Z"]) is ordered.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so this is something you did to please the new linter but the old linter was OK with?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Currently we rely on pyupgrade for this stuff, and it's documented as "timid" about string formatting. ruff is a little less so.

yt/tests/test_funcs.py
desired = "Expected axis to be any of [0, 1, 2, 'x', 'y', 'z', 'X', 'Y', 'Z'], received 'r'"

actual = str(ex.exception)
assert actual == desired
Copy link
Member

Choose a reason for hiding this comment

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

did you manually change this? why?

Copy link
Member Author

Choose a reason for hiding this comment

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

ruff automatically changed a couple strings originally formatted with the % operator to use the str.format method instead. I did a second, manual, pass to use fstrings instead and took the opportunity to slightly improve some affected error messages. Here I'm changing the test to match the -intentionally- updated message.

@matthewturk matthewturk self-requested a review January 31, 2023 17:20
matthewturk
matthewturk reviewed Jan 31, 2023
View reviewed changes
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

Sorry, didn't mean to approve, just meant to leave a deflating comment.

@neutrinoceros
Copy link
Member Author

I'm still pretty ambivalent about switching right now, even if there are other players doing it..

This is fine, but can you be explicit about what conditions should be met for you to be confortable with the migration ?
AFAIC I just don't want to end up with yet another rotting PR.

@matthewturk
Copy link
Member

I think when you will self-describe it as "Stable" rather than just "Used."

I will confess also to having some linting/CI fatigue. I am trying to keep on top of things, but I also really want to see enhancements along other directions as well as linting, and it's hard to keep my mental energy pushing outwards along too many different vectors. So I guess that's what's coloring my ambivalence, but at the same time I don't want to just say no to a (mostly) no-op speedup.

@neutrinoceros
Copy link
Member Author

Perfectly understandable.
For reference, the vision for a stable version of ruff is laid out in the following comment:
astral-sh/ruff#1992 (comment)

I should clarify that what isn't stable is the API, which only affects writing additional code for ruff itself, but the user interface has been extremely stable for a project that hasn't reached version 0.1 yet.

I will confess also to having some linting/CI fatigue. I am trying to keep on top of things, but I also really want to see enhancements along other directions as well as linting, and it's hard to keep my mental energy pushing outwards along too many different vectors.

Somewhat funnily, that's one of my reasons to want to migrate to ruff: it would drastically reduce the number of linting dependencies. I was also thinking, unrelated to ruff, that the time seemed ripe for us to slowdown the pace of pre-commit auto-updates (which I'm happy to do in a separate PR !)

@neutrinoceros neutrinoceros marked this pull request as ready for review January 31, 2023 18:52
@neutrinoceros
Copy link
Member Author

@matthewturk I'll dismiss your review so this doesn't get merged inadvertently.

@neutrinoceros
Copy link
Member Author

I forgot to adapt CONTRIBUTING.rst, switching to draft for now

@neutrinoceros neutrinoceros marked this pull request as draft February 1, 2023 08:55
@neutrinoceros neutrinoceros marked this pull request as ready for review February 1, 2023 11:26
@neutrinoceros neutrinoceros force-pushed the migrate_to_ruff branch 4 times, most recently from 5101d6a to 65e6ab6 Compare February 4, 2023 11:24
@matthewturk
Copy link
Member

I'm coming around. What else would you like to happen?

@neutrinoceros
Copy link
Member Author

I would like @jzuhone to sign off since he also expressed reserves.
All I can add to my case now is that I've switched almost all of my projects to ruff recently and so far it's been nothing but absolute joy.

@jzuhone
Copy link
Contributor

jzuhone commented Feb 10, 2023

I'm ok with this.

@matthewturk matthewturk merged commit 382ccb2 into yt-project:main Feb 15, 2023
@neutrinoceros neutrinoceros deleted the migrate_to_ruff branch February 15, 2023 15:01
@neutrinoceros neutrinoceros mentioned this pull request Feb 21, 2023
Merged
@neutrinoceros neutrinoceros mentioned this pull request May 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewturk matthewturk matthewturk left review comments

@jzuhone jzuhone jzuhone approved these changes

Assignees
No one assigned
Labels
code style Related to linting tools infrastructure Related to CI, versioning, websites, organizational issues, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

STY: migrate to ruff ?
3 participants
@neutrinoceros @matthewturk @jzuhone