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

Lint: Black -> Ruff formatter #14896

Merged

Conversation

ogajduse
Copy link
Member

@ogajduse ogajduse commented Apr 26, 2024

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

  • Remove the black formatter and substitute it with Ruff formatter,
  • bump the version of ruff pre-commit hook.

Related Issues

Related PR: #14781

@ogajduse ogajduse added CherryPick PR needs CherryPick to previous branches 6.12.z Introduced in or relating directly to Satellite 6.12 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 labels Apr 26, 2024
@ogajduse ogajduse self-assigned this Apr 26, 2024
@ogajduse ogajduse requested review from a team as code owners April 26, 2024 11:54
@ogajduse ogajduse force-pushed the lint/black-to-ruff-formatter-n-upgrade branch from 8dc333d to bed6d60 Compare April 26, 2024 13:01
Copy link
Contributor

@synkd synkd left a 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.

tests/foreman/api/test_errata.py Outdated Show resolved Hide resolved
tests/foreman/api/test_errata.py Outdated Show resolved Hide resolved
Copy link
Member

@JacobCallahan JacobCallahan left a 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

Copy link
Contributor

@lhellebr lhellebr left a 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?

@ogajduse
Copy link
Member Author

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.

@jyejare
Copy link
Member

jyejare commented Apr 30, 2024

@ogajduse Could we run some tests before merge it ? Especially the code where reviewers commented/discussed concerns ?

@ogajduse
Copy link
Member Author

@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.

@jyejare
Copy link
Member

jyejare commented May 1, 2024

Sure we can! I do not really see a need to do so

AFAIR, In history, we have broken the sanity when we merged a large formatting PR, So I see the need here.

@ogajduse ogajduse force-pushed the lint/black-to-ruff-formatter-n-upgrade branch from 9d648a7 to 6f771d8 Compare May 2, 2024 09:37
@ogajduse
Copy link
Member Author

ogajduse commented May 2, 2024

trigger: test-robottelo
pytest: tests/foreman/api/test_notifications.py tests/foreman/ui/test_ansible.py::TestAnsibleCfgMgmt::test_positive_assign_ansible_role_variable_on_host -k "long_running_task or test_positive_assign_ansible_role_variable_on_host"

@Gauravtalreja1 Gauravtalreja1 added the AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing label May 2, 2024
@jyejare jyejare merged commit 3f11c65 into SatelliteQE:master May 2, 2024
10 checks passed
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
* Lint: Black -> Ruff formatter

* Address review comments
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
Lint: Black -> Ruff formatter (#14896)

* Lint: Black -> Ruff formatter

* Address review comments
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
ogajduse added a commit to ogajduse/robottelo that referenced this pull request May 3, 2024
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
Gauravtalreja1 pushed a commit that referenced this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.12.z Introduced in or relating directly to Satellite 6.12 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants