GH-121970: Use Ruff to check and format the docs tools by AA-Turner · Pull Request #122018 · python/cpython · GitHub

Issue: Improve and reorganise pyspecific #121970

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

I'm generally in favour!

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

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.

Note there's some overlap with #121241

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

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.

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!

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

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

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

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

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

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

…H-122018) (cherry picked from commit 40855f3) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@gmail.com>

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

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

…ythonGH-122018) (cherry picked from commit 40855f3) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@gmail.com>

) (#122024) (cherry picked from commit 40855f3) Co-authored-by: Alex Waygood <Alex.Waygood@gmail.com>

) (#122023) GH-121970: Use Ruff to check and format the docs tools (GH-122018) (cherry picked from commit 40855f3) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@gmail.com>

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.

Oops, I meant #111192.

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

No problem :)

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?

@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:

(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").

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?

(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.

@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.

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

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/

@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.

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

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.)

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

e2026b9

AA-Turner added docs Documentation in the Doc dir skip news labels Jul 19, 2024

AA-Turner requested review from AlexWaygood and hugovk as code owners July 19, 2024 10:06

bedevere-app Bot mentioned this pull request Jul 19, 2024

Improve and reorganise pyspecific #121970

Open

bedevere-app Bot added the awaiting core review label Jul 19, 2024

AlexWaygood reviewed Jul 19, 2024

View reviewed changes

Comment thread Doc/.ruff.toml Outdated

Comment thread Doc/.ruff.toml Outdated

Comment thread Doc/conf.py Outdated

Comment thread Doc/conf.py Outdated

AlexWaygood reviewed Jul 19, 2024

View reviewed changes

Comment thread .pre-commit-config.yaml

AA-Turner added 4 commits July 19, 2024 11:23

Add ruff-format pre-commit

a882565

Disable E501

13cb3f3

Try pre-commit with '--check'

933095c

Remove flake8-errmsg

a9228fe

AlexWaygood reviewed Jul 19, 2024

View reviewed changes

Comment thread .pre-commit-config.yaml Outdated

AA-Turner added 2 commits July 19, 2024 11:32

Revert the version bump (new Lib/test failures)

efbee68

Update comments in .ruff.toml

7bd87a3

AlexWaygood reviewed Jul 19, 2024

View reviewed changes

Comment thread .pre-commit-config.yaml Outdated

Comment thread .pre-commit-config.yaml Outdated

Comment thread .pre-commit-config.yaml Outdated

Comment thread Doc/.ruff.toml

Comment thread Doc/.ruff.toml

Comment thread Doc/.ruff.toml

Resolve Alex's notes

65ceaa5

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AA-Turner requested a review from AlexWaygood July 19, 2024 10:49

hugovk reviewed Jul 19, 2024

View reviewed changes

Comment thread Doc/.ruff.toml

AA-Turner added 2 commits July 19, 2024 12:01

Resolve Hugo's notes

9e95b25

Merge branch 'main' into docs/use-ruff

8fa92b5

Reformat with new settings

dc85abe

AlexWaygood approved these changes Jul 19, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Jul 19, 2024

Use quoute-style=preserve

a599ef7

miss-islington-app Bot assigned AA-Turner Jul 19, 2024

AA-Turner added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes and removed needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jul 19, 2024

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jul 19, 2024

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jul 19, 2024

AA-Turner added a commit that referenced this pull request Jul 19, 2024

[3.12] GH-121970 : Use Ruff to check and format the docs tools (GH-122018

7c8b6f8

) (#122024) (cherry picked from commit 40855f3) Co-authored-by: Alex Waygood <Alex.Waygood@gmail.com>

hugovk mentioned this pull request Jul 19, 2024

Lint Doc/ with Ruff #111192

Closed

webknjaz reviewed Jul 23, 2024

View reviewed changes

+                      name: Run Ruff (lint) on Argument Clinic
                       args: [--exit-non-zero-on-fix, --config=Tools/clinic/.ruff.toml]
                       files: ^Tools/clinic/|Lib/test/test_clinic.py
+                    - id: ruff-format

GH-121970: Use Ruff to check and format the docs tools by AA-Turner · Pull Request #122018 · python/cpython · GitHub

Uh oh!

Uh oh!

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

GH-121970: Use Ruff to check and format the docs tools#122018AA-Turner merged 13 commits intopython:mainpython/cpython:mainfrom AA-Turner:docs/use-ruffAA-Turner/cpython:docs/use-ruffCopy head branch name to clipboard

Conversation

AA-Turner commented Jul 19, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hugovk commented Jul 19, 2024

Uh oh!

Uh oh!

AA-Turner commented Jul 19, 2024

Uh oh!

AlexWaygood commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app Bot commented Jul 19, 2024

Uh oh!

miss-islington-app Bot commented Jul 19, 2024

Uh oh!

miss-islington-app Bot commented Jul 19, 2024

Uh oh!

miss-islington-app Bot commented Jul 19, 2024

Uh oh!

miss-islington-app Bot commented Jul 19, 2024

Uh oh!

miss-islington-app Bot commented Jul 19, 2024

Uh oh!

bedevere-app Bot commented Jul 19, 2024

Uh oh!

bedevere-app Bot commented Jul 19, 2024

Uh oh!

hugovk commented Jul 19, 2024

Uh oh!

AlexWaygood commented Jul 19, 2024

Uh oh!

hugovk commented Jul 19, 2024

Uh oh!

encukou commented Jul 22, 2024

Uh oh!

AlexWaygood commented Jul 23, 2024

Uh oh!

encukou commented Jul 23, 2024

Uh oh!

AlexWaygood commented Jul 23, 2024

Uh oh!

webknjaz Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

AA-Turner Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

hugovk Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

webknjaz Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

hugovk commented Jul 23, 2024

Uh oh!

encukou commented Jul 27, 2024

GH-121970: Use Ruff to check and format the docs tools#122018
AA-Turner merged 13 commits into
python:mainfrom
AA-Turner:docs/use-ruff

AA-Turner commented Jul 19, 2024 •
edited by github-actions Bot
Loading

AlexWaygood commented Jul 19, 2024 •
edited
Loading