GeezerVison

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.

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

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.

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:

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

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.

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