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

Introduce ruff (eventually replacing autoflake, pyupgrade, flake8) #86224

Merged
merged 7 commits into from
Jan 24, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Jan 19, 2023

Proposed change

This PR adds the extremely fast ruff linter to the pre-commit/lint pipeline; it will probably eventually replace autoflake, pyupgrade, and flake8.

This revision of the PR adheres to the Approach 2 discussed in home-assistant/architecture#863 (comment) aside from adding the VSCode tooling since the version that https://marketplace.visualstudio.com/items?itemName=charliermarsh.ruff ships with is not the same as in the hooks.

Performance

On 3f34871 (dev) it takes about 90 wall-clock seconds on my Macbook to run those tools on the whole repo:

$ time pre-commit run --all-files
pyupgrade................................................................Passed
autoflake................................................................Passed
flake8...................................................................Passed
________________________________________________________
Executed in   92.73 secs    fish           external
   usr time   18.76 mins  338.46 millis   18.76 mins
   sys time    0.36 mins   47.14 millis    0.36 mins

Meanwhile, ruff (with the configuration in this PR), with a freshly cleaned cache, takes 8 seconds. Yes, it really is that fast.

$ time pre-commit run --all-files ruff
ruff.....................................................................Passed
________________________________________________________
Executed in    8.01 secs    fish           external
   usr time   21.22 secs    0.12 millis   21.22 secs
   sys time    2.49 secs    4.68 millis    2.49 secs

Nota bene

  • The configured exclusions were set to minimize the number of changes required to the current code; the few remaining actual code changes were actual issues found by ruff that hadn't been spotted by the previous linting tools. Exclusions could be peeled off if required, likely in follow-up PRs.
  • ruff has partial support for pylint, bandit, and isort rules, but those aren't enabled in this PR yet out of an abundance of caution due to the custom configuration in the repo.

Type of change

  • Code quality improvements to existing code or addition of tests

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.
    • No new code.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.

@home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (websocket_api) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of websocket_api can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign websocket_api Removes the current integration label and assignees on the issue, add the integration domain after the command.

@home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (light) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of light can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign light Removes the current integration label and assignees on the issue, add the integration domain after the command.

@home-assistant
Copy link

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (bluetooth) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of bluetooth can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign bluetooth Removes the current integration label and assignees on the issue, add the integration domain after the command.

@home-assistant
Copy link

Hey there @emontnemery, @jbouwh, mind taking a look at this pull request as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mqtt can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign mqtt Removes the current integration label and assignees on the issue, add the integration domain after the command.

@home-assistant
Copy link

Hey there @OttoWinter, @jesserockz, mind taking a look at this pull request as it has been labeled with an integration (esphome) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of esphome can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign esphome Removes the current integration label and assignees on the issue, add the integration domain after the command.

@akx akx marked this pull request as ready for review January 23, 2023 10:20
pyproject.toml Outdated Show resolved Hide resolved
@frenck frenck mentioned this pull request Jan 23, 2023
19 tasks
@akx akx requested review from frenck and removed request for bdraco, jesserockz, OttoWinter, jbouwh and emontnemery January 23, 2023 18:56
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@akx

This comment was marked as outdated.

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Alright! Looks ready to go, can be merged once the CI passes.

Thanks @akx 👍

Let's see how it goes and re-evaluate the next steps after some experience has been gained.

../Frenck

akx added a commit to akx/ruff that referenced this pull request Jan 24, 2023
@frenck frenck merged commit bf41a97 into home-assistant:dev Jan 24, 2023
@akx akx deleted the ruff branch January 24, 2023 11:41
@akx akx mentioned this pull request Jan 24, 2023
16 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.