Skip to content

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Oct 19, 2024

https://docs.astral.sh/ruff

Ruff can be used to replace Flake8 (plus dozens of plugins), Black, isort, pydocstyle, pyupgrade, autoflake, and more, all while executing tens or hundreds of times faster than any individual tool.

@JoelLefkowitz
Copy link
Collaborator

This is interesting, I've never heard of ruff before. Speed for the static checkers hasn't been a concern for this repository before, but the simpler configuration and invocation of ruff looks really good

@JoelLefkowitz
Copy link
Collaborator

I also like the idea of separating the linters from tox, since it's meant to be a testing tool

@cclauss
Copy link
Contributor Author

cclauss commented Oct 21, 2024

Many projects put subsecond tools like codespell, ruff, etc into https://pre-commit.com
Jupyter: https://github.com/jupyter/notebook/blob/main/.pre-commit-config.yaml

@JoelLefkowitz JoelLefkowitz added 1.21.x Release target in 1.21.x enhancement Enhancement labels Mar 7, 2025
@cclauss
Copy link
Contributor Author

cclauss commented Mar 7, 2025

Scarry to run tests on Python 3.6 but not Python 3.13.

@JoelLefkowitz
Copy link
Collaborator

JoelLefkowitz commented Mar 7, 2025

Scarry to run tests on Python 3.6 but not Python 3.13.

We stopped the python3.6 tests a while ago, but I don't have enough permissions on the repo to remove it as a required check:

# Python 3.6 tests have been removed since swagger-spec-validator no longer supports it.
# A successful workflow run for Python 3.6 is required by the GitHub branch protection rules.
if: ${{ matrix.python != 3.6 }}

Now it just skips them

@JoelLefkowitz
Copy link
Collaborator

JoelLefkowitz commented Mar 7, 2025

Thanks for this PR @cclauss,

I've had a look at the branch and would like to simplify the pyproject.toml a little:

[build-system]
build-backend = "setuptools.build_meta"
requires = ["setuptools>=68", "setuptools-scm>=3.0.3", "wheel"]

[tool.ruff]
exclude = ["**/migrations/*"]
line-length = 80

lint.select = [
    "ASYNC", # flake8-async
    "C4",    # flake8-comprehensions
    "C90",   # mccabe
    "E",     # pycodestyle
    "F",     # pyflakes
    "FA",    # flake8-future-annotations
    "FLY",   # flynt
    "I",     # isort
    "ICN",   # flake8-import-conventions
    "INT",   # flake8-gettext
    "LOG",   # flake8-logging
    "PLE",   # pylint
    "PYI",   # flake8-pyi
    "SLOT",  # flake8-slots
    "T10",   # flake8-debugger
    "TCH",   # flake8-type-checking
    "W",     # pycodestyle warnings
    "YTT",   # flake8-2020
]

lint.isort.known-first-party = [
    "articles",
    "drf_yasg",
    "people",
    "snippets",
    "testproj",
    "todo",
    "urlconfs",
    "users",
]

lint.mccabe.max-complexity = 13
lint.per-file-ignores."src/drf_yasg/inspectors/field.py" = ["E721"]
lint.per-file-ignores."src/drf_yasg/openapi.py" = ["E721"]
lint.per-file-ignores."testproj/testproj/settings/*" = ["F405"]

After this PR is merged I'd like to run the formatter in a new PR. The code is currently not compliant with any formatter. When we run ruff format most of the source folder will be modified. I've checked whether setting the quotes to 'single' would have less impact but it still updates about 50 files. The line width we choose comes down to opinion and I suggest we stick with the 80 characters limit that is the standard, even though I know lots of people don't like it.

Other related things that I'd like to do in separate PRs:

  • Migrate the requirements files to the pyproject.toml's [project.optional-dependencies]
  • Remove the linters from the tox.ini since it should be responsible for testing only
  • Add a linting step to the review workflow (for the latest python version only)

@cclauss
Copy link
Contributor Author

cclauss commented Mar 7, 2025

line-length = 80 instead of the default 88?

@JoelLefkowitz
Copy link
Collaborator

The line-too-long (E501) rule suggests 88, but the PEP 8 suggestion is 80. The ruff docs say they use 88 for compatibility with Black and the Ruff formatter. It's arbitrary which one we pick since they're both followed by large projects. I checked the Django repository and they're using 88. I think we should stick to the PEP 8 convention though.

@cclauss
Copy link
Contributor Author

cclauss commented Mar 7, 2025

Please read the justification for the change in psf/black docs.

@JoelLefkowitz
Copy link
Collaborator

@cclauss is this the section you mean?

You probably noticed the peculiar default line length. Black defaults to 88 characters per line, which happens to be 10% over 80. This number was found to produce significantly shorter files than sticking with 80 (the most popular), or even 79 (used by the standard library). In general, 90-ish seems like the wise choice.

Although shorter files is certainly a good thing I think that sticking to the most popular length is the smarter choice. My preference is 120 but I'd much rather just stick to PEP 8 because that aligns with what a majority of people are used to and we get a lot of contributors.

@cclauss
Copy link
Contributor Author

cclauss commented Mar 7, 2025

My point is that since black and ruff format have taken over the automated Python formatting space, MOST Python codebases are 88. Checkout NumPy, SciPy, Jupyter, Pyodide, Django, FastAPI, Pydantic, etc. I bet that in really popular codebases it will be difficult to find 80 char limits anymore.

@JoelLefkowitz
Copy link
Collaborator

I found this stack overflow answer pretty convincing when I researching what the standard is:

Much of the value of PEP-8 is to stop people arguing about inconsequential formatting rules, and get on with writing good, consistently formatted code. Sure, no one really thinks that 79 is optimal, but there's no obvious gain in changing it to 99 or 119 or whatever your preferred line length is. I think the choices are these: follow the rule and find a worthwhile cause to battle for, or provide some data that demonstrates how readability and productivity vary with line length. The latter would be extremely interesting, and would have a good chance of changing people's minds I think.

@cclauss
Copy link
Contributor Author

cclauss commented Mar 7, 2025

It was written 15 years ago and does represent the way things were in that epoch. Things have changed in the interim.

@JoelLefkowitz
Copy link
Collaborator

Tbh I really don't mind as long as we stick to it. If it's the default for ruff format then lets keep 88, could you please update the PR?

@JoelLefkowitz
Copy link
Collaborator

@cclauss could you please update the PR to 88?

@cclauss cclauss force-pushed the ruff branch 2 times, most recently from bff535c to d1eee24 Compare March 17, 2025 12:49
@JoelLefkowitz
Copy link
Collaborator

Thank you @cclauss

@JoelLefkowitz JoelLefkowitz merged commit 6997457 into axnsan12:master Mar 17, 2025
7 checks passed
@cclauss cclauss deleted the ruff branch March 17, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.x Release target in 1.21.x enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants