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

Bump ruff to 0.9.2 #3743

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Bump ruff to 0.9.2 #3743

wants to merge 1 commit into from

Conversation

abmantis
Copy link
Contributor

@abmantis abmantis commented Jan 20, 2025

Proposed change

SSIA

Additional information

Checklist

  • The changes are tested and work correctly
  • pre-commit checks pass / the code has been formatted using Black
  • Tests have been added to verify that the new code works

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.01%. Comparing base (edf9432) to head (0072476).
Report is 5 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #3743   +/-   ##
=======================================
  Coverage   90.01%   90.01%           
=======================================
  Files         320      320           
  Lines       10411    10411           
=======================================
  Hits         9371     9371           
  Misses       1040     1040           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@TheJulianJES TheJulianJES 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 not sure if we even need this in requirements_test.txt at all anymore.
We use the ruff hook for pre-commit now (.pre-commit-config.yaml), so we might be able to drop this completely from requirements_test.txt.

(There's an open PR to update the ruff hook from 0.8.3 to 0.9.2 here.)

@TheJulianJES TheJulianJES added the code quality Improvement to code quality label Jan 21, 2025
@abmantis
Copy link
Contributor Author

The downside of not having it on requirements_test.txt is that then it will use the system ruff during development which may be a different version than the one we have on pre-commit.

@prairiesnpr
Copy link
Collaborator

The downside of not having it on requirements_test.txt is that then it will use the system ruff during development which may be a different version than the one we have on pre-commit.

I'm also in favor of keeping ruff in requirements_test.txt and keeping it in sync with pre-commit.

@TheJulianJES
Copy link
Collaborator

The downside of not having it on requirements_test.txt is that then it will use the system ruff during development

So when using the Ruff extension for VS code for example? (I'm mainly not using VS Code, so not sure about this)

pre-commit run --all-files automatically fixes/reformats all files and uses the version from the pre-commit config (that's also what the local Git hook automatically does for the current file and what our CI does).


In the zigpy repo, we do not include ruff at all: https://github.com/zigpy/zigpy/blob/dev/requirements_test.txt, same with the uiprotect library for example.
In the ZHA repo, it's still included for some reason, but not pinned at a specific version. The CI/tests do not need it there anymore either, so it should be removed too IMO.

The automatic PRs from the pre-commit CI GitHub app will also only update the pre-commit config, not requirements_test.txt. That's why I'd rather drop the requirement completely, unless there's good reason otherwise, as we'll always need to manually update keep both in sync otherwise.

@abmantis
Copy link
Contributor Author

So when using the Ruff extension for VS code for example? (I'm mainly not using VS Code, so not sure about this)

In my case with the ruff lsp on neovim, but the same should apply to vscode.

The automatic PRs from the pre-commit CI GitHub app will also only update the pre-commit config, not requirements_test.txt. That's why I'd rather drop the requirement completely, unless there's good reason otherwise, as we'll always need to manually update keep both in sync otherwise.

Could removing the installation from pre-commit and relying on requirements_test.txt for dependency setup be an option? I.e., make the CI do pip install -r requirements_test.txt and pre-commit will only run the checks (but not install anything)?

I am also fine with just removing it from requirements_test.txt, but would prefer to have it as part of the dev environment. In the past I got confused as to why my editor was formatting files (using ruff) differently from the pre-commit, before realizing they were using different versions. But again, fine with dropping it too.

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

Successfully merging this pull request may close these issues.

3 participants