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

pre-commit: Replace flake8, isort, and pyupgrade with ruff #584

Closed
wants to merge 3 commits into from
Closed

pre-commit: Replace flake8, isort, and pyupgrade with ruff #584

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4200898
pre-commit: Replace flake8, isort, and pyupgrade with ruff
cclauss Feb 28, 2023
430f0c8
Make changes requested in code review
cclauss Mar 20, 2023
469085c
Add ruff per-file-ignores
cclauss Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 4 additions & 12 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,8 @@ repos:
- id: end-of-file-fixer
exclude: '.*\.pth$'
- id: debug-statements
- repo: https://github.com/PyCQA/isort
rev: 5.12.0
- repo: https://github.com/charliermarsh/ruff-pre-commit
rev: v0.0.253
hooks:
- id: isort
- repo: https://github.com/asottile/pyupgrade
rev: v3.3.1
hooks:
- id: pyupgrade
args: [--py37-plus]
- repo: https://github.com/PyCQA/flake8
rev: 6.0.0
hooks:
- id: flake8
- id: ruff
# args: [--fix, --exit-non-zero-on-fix]
cclauss marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 26 additions & 0 deletions .ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# [tool.ruff] # <- TODO Uncomment when migrating to pyproject.toml
exclude = [
".eggs",
".tox",
"build",
"ci/templates",
"dist",
]
line-length = 140
select = [
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be extend-select instead?

Copy link
Contributor Author

@cclauss cclauss Mar 10, 2023 •
edited

Choose a reason for hiding this comment

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

If you're wondering how to configure Ruff, here are some recommended guidelines:

  • Prefer select and ignore over extend-select and extend-ignore, to make your rule set explicit.

https://beta.ruff.rs/docs/configuration

Copy link
Member

Choose a reason for hiding this comment

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

Well all I want is to not hardcode a ruleset (which select = [stuff] exactly is). I don't think any of those recommendations are worth considering blindly given the tool's age and popularity.

How about select = ["ALL"] and ignore whatever doesn't make sense for pytest-cov?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're wondering how to configure Ruff, here are some recommended guidelines:

  • Prefer select and ignore over extend-select and extend-ignore, to make your rule set explicit.
  • Use ALL with discretion. Enabling ALL will implicitly enable new rules whenever you upgrade.
  • Start with a small set of rules (select = ["E", "F"]) and add a category at-a-time. For example, you might consider expanding to select = ["E", "F", "B"] to enable the popular flake8-bugbear extension.
  • By default, Ruff's autofix is aggressive. If you find that it's too aggressive for your liking, consider turning off autofix for specific rules or categories (see FAQ).

Copy link
Member

Choose a reason for hiding this comment

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

There are way too many categories to enable manually. I'd expect that there are more good checks than broken or bad ones.

I noticed it's aggressive ... or rather destructive. That's why I think --show-fixes is a must. I think avoiding --fix actually adds more friction (too much back and forth for avoidable manual stuff).

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 I've played with it a bit more.... now I am not really sure what the best way... eg: these are either invalid checks or would be a pain to sort out, or not even worth fixing:

ignore = [
    "D", # pydocstyle
    "ANN", # flake8-annotations
    "ARG", # flake8-unused-arguments
    "BLE", # flake8-blind-except
    "C90", # mccabe
    "EM", # flake8-errmsg
    "FBT", # flake8-boolean-trap
    "INP", # flake8-no-pep420
    "PLR", # Pylint Refactor
    "PLW", # Pylint Warning
    "Q", # flake8-quotes
    "RET", # flake8-return
    "SIM102", # flake8-simplify collapsible-if
    "SIM105", # flake8-simplify use-contextlib-suppress
    "SLF", # flake8-self
    "T20", # flake8-print
    "S101", # flake8-bandit assert
    "S102", # flake8-bandit exec
    "S110", # flake8-bandit try-except-pass
    "TRY", # tryceratops
]

Now considering that rsync features are being dropped in xdist ... a lot of bulk using that in pytest-cov becomes unnecessary, if it isn't already as some people are suggesting pytest-cov is not necessary for getting xdist support...

I guess select with a few is fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... How should we proceed? Given your explorations and your knowledge of the codebase, do you want to open a new PR that cherry-picks from this one? I have no trouble closing this with unmerged commits. Or should I press on?

Copy link

Choose a reason for hiding this comment

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

I'd also recommend the C4 checks as they are popular, increase performance, and have autofixes enabled for all rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there somewhere proof that C4 mods increase performance?

"E",
"F",
"I",
"PLC",
"PLE",
"UP",
"W",
]
target-version = "py37"

# [tool.ruff.isort] # <- TODO Uncomment when migrating to pyproject.toml
cclauss marked this conversation as resolved.
Show resolved Hide resolved
[isort]
# default-section = "THIRDPARTY"
force-single-line = true
forced-separate = ["test_pytest_cov"]
known-first-party = ["pytest_cov"]
8 changes: 4 additions & 4 deletions MANIFEST.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,22 @@ prune examples/adhoc-layout/*.egg-info
prune examples/src-layout/src/*.egg-info

graft .github/workflows
graft src
graft ci
graft src
graft tests

include .bumpversion.cfg
include .cookiecutterrc
include .coveragerc
include .editorconfig
include tox.ini
include .readthedocs.yml
include .pre-commit-config.yaml
include .readthedocs.yml
include .ruff.toml
include AUTHORS.rst
include CHANGELOG.rst
include CONTRIBUTING.rst
include LICENSE
include README.rst

include tox.ini

global-exclude *.py[cod] __pycache__/* *.so *.dylib