-
Notifications
You must be signed in to change notification settings - Fork 115
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
Lint: Black -> Ruff formatter #14896
Lint: Black -> Ruff formatter #14896
Conversation
8dc333d
to
bed6d60
Compare
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.
A couple of small, non-blocking suggestions, but looks good to me overall.
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.
ACK. Merge pending Danny's recommendations
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 think this is OK but is there some specific reason for us to prefer Ruff?
Astral's page sums it up pretty well. https://astral.sh/blog/the-ruff-formatter I would like us to switch to Ruff mainly because of its speed compared to black. It is incredibly fast and the compatibility with Black is 99%. Looking at the patch, it might not look like 99%, but taking into account how big our project is and what changes were made, it is not that bad. |
@ogajduse Could we run some tests before merge it ? Especially the code where reviewers commented/discussed concerns ? |
Sure we can! I do not really see a need to do so, but @Gauravtalreja1 and/or @shweta83, feel free to trigger PRT here. |
AFAIR, In history, we have broken the sanity when we merged a large formatting PR, So I see the need here. |
9d648a7
to
6f771d8
Compare
trigger: test-robottelo |
* Lint: Black -> Ruff formatter * Address review comments
* Lint: Black -> Ruff formatter * Address review comments
Problem Statement
#14781 attempts to upgrade some of our pre-commit hooks including black. The new version of black formats 150+ files. Given the patch size, I propose switching to Ruff formatter as the patch size is comparable to the patch for the new version of black.
Reason to switch to Ruff
(copy&paste from one of the comments in this PR)
Astral's page sums it up pretty well. astral.sh/blog/the-ruff-formatter
I would like us to switch to Ruff mainly because of its speed compared to black. It is incredibly fast and the compatibility with Black is 99%. Looking at the patch, it might not look like 99%, but taking into account how big our project is and what changes were made, it is not that bad.
Solution
Related Issues
Related PR: #14781