GH-121970: Use Ruff to check and format the docs tools

[AA-Turner]

• Issue: Improve and reorganise pyspecific #121970

---

📚 Documentation preview 📚: https://cpython-previews--122018.org.readthedocs.build/

[AlexWaygood]

I'm generally in favour!

[hugovk]

I'm generally in favour!

Me too! Note there's some overlap with #121241 which @AlexWaygood wanted to post about on Discourse first. I'm also fine with not doing so.

[AA-Turner]

Note there's some overlap with #121241

Is that the right issue/PR? I don't see a mention of Ruff on it.

[AlexWaygood]

I'm specifically nervous about enabling more aggressive linting on stdlib modules in Lib/, as I think it's important we get consensus among core devs before going ahead with that more broadly. For stuff in Tools/ and Doc/, I think it's fine to enable as much linting/formatting as we like. This is all purely internal tooling, so the stakes are lower, the stakeholders are fewer, keeping a clean git blame is less important, etc.

[AlexWaygood]

I would personally go for quote-style = "preserve", and switch to quote-style = "double" in a followup, rather than going for quote-style = "single" (https://github.com/python/cpython/pull/122018/files#r1684185495). But I don't feel strongly. LGTM!

[miss-islington-app]

Thanks @AA-Turner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @AA-Turner, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 40855f3ab80ced9950c725b710f507c0e903b70a 3.13

[miss-islington-app]

Sorry, @AA-Turner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 40855f3ab80ced9950c725b710f507c0e903b70a 3.12

[miss-islington-app]

Thanks @AA-Turner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[miss-islington-app]

Thanks @AA-Turner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @AA-Turner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 40855f3ab80ced9950c725b710f507c0e903b70a 3.12

[bedevere-app]

GH-122023 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-122024 is a backport of this pull request to the 3.12 branch.

[hugovk]

Note there's some overlap with #121241

Is that the right issue/PR? I don't see a mention of Ruff on it.

Oops, I meant #111192.

[AlexWaygood]

Oops, I meant #111192.

Oops 😶 sorry for forgetting about that PR and for being inconsistent about my opinion on this one @hugovk!!

[hugovk]

No problem :)

[encukou]

I got a lint failure on a PR:

Ruff (I001)

Doc/tools/version_next.py:15:1: I001 Import block is un-sorted or un-formatted

As far as I know, the import block is OK according to PEP 8.
The error message doesn't hint at how to fix it if there is a problem, nor how to silence the error if it's a false positive.

When adding checks like this, could you also add some instructions to the devguide about how to deal with the warnings -- preferably without installing third-party tools I might not trust?

[AlexWaygood]

@encukou, I'm sorry if we rushed this. Sorry that the error was inscrutable; thanks for flagging.

could you also add some instructions to the devguide

We already have documentation in the devguide noting that we have pre-commit checks set up, and giving some instructions about setting up pre-commit locally:

• https://devguide.python.org/getting-started/pull-request-lifecycle/#step-by-step-guide
• https://devguide.python.org/getting-started/setup-building/#install-pre-commit-as-a-git-hook

(I personally tend to avoid actually installing pre-commit hooks myself -- I usually run pre-commit run -a to fix all linting issues.)

preferably without installing third-party tools I might not trust

Our CI is setup so that generally pre-commit will show you the changes it wants you to make to your PR as a diff when CI fails. In this case, it looks like that didn't happen for #121278. I'm not sure why that is, but anyway -- it's telling you that your imports aren't alphabetically sorted. You can find the documentation for the I001 Ruff rule here (I googled "Ruff I001").

[encukou]

We already have documentation in the devguide noting that we have pre-commit checks set up, and giving some instructions about setting up pre-commit locally

Thanks! I filed python/devguide#1360 to improve the set-up instructions.

(If you don't do it yourself, is installing pre-commit as a commit hook the right thing to do?)

it's telling you that your imports aren't alphabetically sorted

I don't understand why it is important to sort imports alphabetically.
If it is best practice, shouldn't we mention it in PEP 8?

[AlexWaygood]

(If you don't do it yourself, is installing pre-commit as a commit hook the right thing to do?)

It's a thing that many people like to do. The advantage is that you don't have to run the linter manually; it automatically applies any autofixes as part of running git commit. The disadvantage is that you don't have to run the linter manually -- automatically applying autofixes as part of git commit is a bit too magical for my tastes. I prefer to see explicitly what changes the autofixer has made when I'm working on a patch locally. Running pre-commit run -a explicitly runs all linters that we have listed in our .pre-commit-config.yaml file, and that's what I prefer.

I wasn't involved in the decision to add a section to the devguide telling people to setup commit hooks to run pre-commit.

I don't understand why it is important to sort imports alphabetically.

It's not particularly important, but many people have decided that it's something they want to enforce as a matter of style. I personally like to enforce a consistent style of imports in this way in the projects I maintain, but I also wouldn't be distraught if we got rid of this rule.

If it is best practice, shouldn't we mention it in PEP 8?

PEP 8 is the style guide for the standard library, and we're not enforcing this rule on the standard library -- only internal tooling in the Doc/ directory.

[webknjaz]

@AA-Turner in pre-commit, it's usually best to put formatters before linters. This is because if there's a rule violation that linter shows, it will then show up as two different broken checks. It's also more important for the post-format version to be checked rather than showing errors on things that then get changed/relocated.

[AA-Turner]

Oh, are the pre-commit tools not idempotent? I hadn't realised that order mattered, as I thought we'd configured them to check only in a read-only fashion.

As you can probably tell, I don't use pre-commit personally!

A

[hugovk]

The Ruff pre-commit README says:

When running with --fix, Ruff's lint hook should be placed before Ruff's formatter hook, and before Black, isort, and other formatting tools, as Ruff's fix behavior can output code changes that require reformatting.

When running without --fix, Ruff's formatter hook can be placed before or after Ruff's lint hook.

https://github.com/astral-sh/ruff-pre-commit/

[webknjaz]

@AA-Turner oh, I didn't realize. It's so popular that I tend to forget there's people who don't use it. I think I first contributed to pre-commit in 2016…

But yeah, there's science to crafting a config for the pre-commit tool. And there are a few principles I found very useful in this context. Here's how I sort the entries:

• fast checks first, followed by heavier linters (as in, flake8 goes before pylint, for example, by MyPy would probably be in between)
• formatters before linters (as in, autopep8 and then flake8)
• if there are groups of formatters+linters touching different file categories, the entire category can be moved together, no need to put autopep8 and docformatter together as they change different file types

pre-commit itself does not modify things by itself, so you can say it's idempotent in this sense. It's the checks that call other tools that modify files. I think if a check changes files (pre-commit docs sometimes call these “fixers”, not “formatters”), pre-commit will then show that entry as failed (although, it probably relies on the called program's return code). When something changed, it's useful to know what, so it's best to call the tool with --show-diff-on-failure. I think the https://pre-commit.ci web service does this by default (this service is also able to push a commit with the changed files back to pull requests, by the way). Some people only use this CLI flag in CI, while others stick it into the command runner like tox so it's active locally when the contributors run linting.

Feel free to tag me for reviews in future PRs if you need any more insight on this topic.

[hugovk]

I don't understand why it is important to sort imports alphabetically.

It's not particularly important, but many people have decided that it's something they want to enforce as a matter of style. I personally like to enforce a consistent style of imports in this way in the projects I maintain, but I also wouldn't be distraught if we got rid of this rule.

Same here on both counts.

Alphabetical import sorting can be more useful, than for example, new imports being tacked onto the end:

• If there's lots of imports, alphabetical is quicker to scan by eye and find whether a given import is already there.

• It prevents duplicate imports being added (maybe because you didn't see it already there due to arbitrary order).

• It can help prevent merge conflicts, where two people tack on new imports at the end at the same time.

• Files are easier to diff and see which have common imports when they are sorted alphabetically, compared to arbitrarily or chronological order.

• Finally, isort also helps us follow PEP 8:

  • puts one import per line
  • groups them into stdlib/third-party/local groups

[encukou]

I personally like to enforce a consistent style of imports in this way in the projects I maintain, but I also wouldn't be distraught if we got rid of this rule.

But as a contributor to various projects, each of which expects or enforces a slightly different code style (and most of which expect me to format the code), I get the opposite of consistency.
Here, we have inconsistent styles within a single repo.

If alphabetical import sorting is useful, I do think it should be preferred for new code across the entire project. Then, it would be easier to accept that in some parts of the code we enforce some parts of the style guide.
That's what I was going for with “shouldn't we mention it in PEP 8?”

---

It worries me to see a maintainer that has things set up just right, slightly differently than what's in the docs, while others get an inscrutable error for what should be a simple contribution. Should we be worried about non-dogfooded devguide instructions?
(In this case, it's not easy to derive “your imports aren't alphabetically sorted” even from the I001 docs.)