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

Consider switching to ruff for linting #12072

Closed
1 task done
pfmoore opened this issue Jun 5, 2023 · 3 comments · Fixed by #12073
Closed
1 task done

Consider switching to ruff for linting #12072

pfmoore opened this issue Jun 5, 2023 · 3 comments · Fixed by #12073
Labels
S: needs triage Issues/PRs that need to be triaged type: feature request Request for a new feature

Comments

@pfmoore
Copy link
Member

pfmoore commented Jun 5, 2023

What's the problem this feature will solve?

We currently use flake8 for our linting. There's nothing wrong with that, but ruff is a lot faster, as well as being a single tool rather than one tool plus some (independently maintained) plugins.

The set of warnings is going to be slightly different (ruff doesn't claim to implement everything included in the various plugins it copies) but I'm not sure we care as long as the results on the current codebase are the same - I don't believe anyone ever audited the full list of checks currently applied1, our style rules are basically just "must pass the tools we use in our standard linting config". On which basis, changing the linter seems perfectly acceptable.

Describe the solution you'd like

Replace our existing flake8 configuration with ruff. We could also use ruff in place of isort, further simplifying our setup.

Alternative Solutions

Stick with what we have. It's not as if there's a great need to change.

To be clear here, I'm not pushing for this, I'm raising this mostly just to get a feel for what others think of the idea.

Additional context

I did some experimenting, and the following addition to pyproject.toml seems to replicate what we have for flake8.

[tool.ruff]
exclude = [
    "./build",
    ".nox",
    ".tox",
    ".scratch",
    "_vendor",
    "data",
]
ignore = [
    "B019",
    "B020",
    "B904", # Ruff enables opinionated warnings by default
    "B905", # Ruff enables opinionated warnings by default
    "G202",
]
line-length = 88
select = [
    "B",
    "E",
    "F",
    "W",
    "G",
    "ISC",
]

[tool.ruff.per-file-ignores]
"noxfile.py" = ["G"]
"tests/*" = ["B011"]

There are two warnings remaining:

src\pip\_internal\network\auth.py:517:9: B018 Found useless expression. Either assign it to a variable or remove it.
tests\lib\__init__.py:687:39: B026 Star-arg unpacking after a keyword argument is strongly discouraged

To be honest, both seem legitimate (although it's a matter of opinion whether we should fix or ignore them, I don't really have a preference) and I'm not clear why flake8-bugbear doesn't flag them - I couldn't see any indication that they are disabled by default.

Code of Conduct

Footnotes

  1. Much less review each new linter release to decide if we like whatever gets changed 🙂

@pfmoore pfmoore added type: feature request Request for a new feature S: needs triage Issues/PRs that need to be triaged labels Jun 5, 2023
@pfmoore
Copy link
Member Author

pfmoore commented Jun 5, 2023

Also, as a result of globality-corp/flake8-logging-format#68, I expect our current lint setup to start failing on Python 3.12 (I first started thinking about using ruff because I hit that problem locally, as I configure virtualenv to omit setuptools and wheel by default).

@pradyunsg
Copy link
Member

Let's do it. :)

@notatallshaw
Copy link
Member

I'm a big fan of Ruff, I will point out a couple of things to be aware of:

Last I checked parsing stack is based on RustPython which uses an LL(1) parser rather than a PEG parser, this can lead to them to implementing new syntax rather slowly. E.g. match statement syntax has only been supported since February 2023 even though it had been in Python since October 2021.

Also Ruff is relatively new and may be missing edge cases. E.g. I discovered recently it's scoping rules inside comprehensions inside class scope was wrong (I found this as I was testing PEP 709 discussion), I reported and the author was was very quick to fix.

Neither of these things I think should significantly affect Pip, but something to be aware of.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
S: needs triage Issues/PRs that need to be triaged type: feature request Request for a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants