Sort imports for more consistent diff (ruff/isort) #4392

Avasam · 2024-05-23T23:04:14Z

Summary of changes

As title says.
I've made sure that _distutils_hack should be sorted before _distutils, and that setuptools gets sorted before distutils in tests as long as the hack is supported (ref: #4137 )

Pull Request Checklist

Changes have tests
News fragment added in newsfragments/. (no user-facing changes)
(See documentation for details)

ruff.toml

Avasam · 2024-07-20T18:39:35Z

pkg_resources/__init__.py

+from os import open as os_open, utime  # isort: skip
+from os.path import isdir, split  # isort: skip


This change is not automated

Avasam · 2024-07-20T18:42:07Z

setuptools/dist.py

+from . import (
+    _entry_points,
+    _reqs,
+    command as _,  # noqa: F401 # imported for side-effects


This change is not automated (made the noqa more specific)

Avasam · 2024-07-20T18:50:01Z

setuptools/tests/config/test_pyprojecttoml.py

@@ -4,21 +4,19 @@

 import jaraco.path
 import pytest
+import setuptools  # noqa: F401 # force distutils.core to be patched


This change is not automated (more precise noqa)

Avasam · 2024-07-20T18:50:27Z

setuptools/tests/test_config_discovery.py

@@ -3,18 +3,17 @@
 from configparser import ConfigParser
 from itertools import product

+import jaraco.path
+import pytest
+import setuptools  # noqa: F401 # force distutils.core to be patched


This change is not automated (more precise noqa)

abravalheri

Thank you very much @Avasam.

I think in the case of this PR it would be interesting in separating 2 commits: 1 for the configuration changes (like ruff.toml and .pre-commit-config.yaml) and another one for the bulk of the automated changes. If necessary git commit --no-verify can be used to allow the first commit to be made without changing the rest of the codebase.

Because squashing is not viable if we want to keep the 2 commits separated, it would make more sense to use rebase instead of merging if it is necessary to update the codebase. This also implies in more coordination with the other PRs for the best moment/order to merge, but I think we can do that.

abravalheri · 2024-08-09T16:20:12Z

ruff.toml

+known-third-party = ["setuptools"]
+known-first-party = ["distutils"]


Would this make sense?

Suggested change

known-third-party = ["setuptools"]

known-first-party = ["distutils"]

section-order = ["future", "standard-library", "eager", "third-party", "first-party", "local-folder", "delayed"]

sections.eager = ["_distutils_hack"]

sections.delayed = ["distutils"]

It is more verbose, but also more compatible with what isort would do by default, special-casing only distutils and _distutils_hack

I didn't realize I could create custom sections. That is so much better yes !

.pre-commit-config.yaml

Avasam · 2024-08-09T19:19:16Z

I think in the case of this PR it would be interesting in separating 2 commits

Certainly, now that I have confirmation this is indeed wanted, I'll be cautious about keeping it into 2 clean commits.

implies in more coordination with the other PRs for the best moment/order to merge, but I think we can do that.

Hence I've been lazily merging conflicts until now ^^ (with an occasional soft reset)

abravalheri

Thank you very much @Avasam for putting so much work on this.

I am interested in merging this one before anything else, to get it out of the way and rip off the metaphorical band-aid. Unless you have a different suggestion.

abravalheri · 2024-08-12T10:03:00Z

ruff.toml

+# Force Ruff/isort to always import setuptools before distutils in tests as long as distutils_hack is supported
+# This also ensures _distutils_hack is imported before distutils
+# https://github.com/pypa/setuptools/issues/4137
+section-order = ["future", "standard-library", "eager", "third-party", "first-party", "delayed", "local-folder"]


I propose we leave "delayed" as the last item in this array.

Suggested change

section-order = ["future", "standard-library", "eager", "third-party", "first-party", "delayed", "local-folder"]

section-order = ["future", "standard-library", "eager", "third-party", "first-party", "local-folder", "delayed"]

The reasoning is the following:

Relative import statements like from . import .. seem to be considered "local-folder", but they still refer to setuptools. This means that the configuration ... "delayed", "local-folder"] will place distutils imports before setuptools imports, but that is what we are trying to avoid. So my recommendation is to write ..., "local-folder", "delayed"].

For example ... "delayed", "local-folder"] change setuptools/command/bdist_wheel.py to:

from packaging import tags, version as _packaging_version from wheel.metadata import pkginfo_to_metadata from wheel.wheelfile import WheelFile from distutils import log from .. import Command, __version__ from .egg_info import egg_info as egg_info_cls

But we would like:

from packaging import tags, version as _packaging_version from wheel.metadata import pkginfo_to_metadata from wheel.wheelfile import WheelFile from .. import Command, __version__ from .egg_info import egg_info as egg_info_cls from distutils import log

Here's my reasoning:
For non-setuptools folders, like tools and tests, it doesn't change anything and it's closer to what a user of setuptools would have.

For setuptools itself: AFAIK, to import a submodule, the top module/package gets imported too. So _distutils_hack should still always run first. Unless I'm missing something:

(overly zealous prints but I'm trying to remove assumptions from my proof)

It was also already that way for most imports.

_distutils_hack will not be affected by putting delayed last, right? (Because _distutils_hack goes to its own group: eager, which always comes first).

So if that is the concern, "local-folder", "delayed"] should be fine.

As long as eager is before first-party, local-folder and delayed yeah

Just to be clear, I think swapping the order of "local-folder" and "delayed" works in both cases. I'm arguing for keeping the expectation and habit of putting relative imports last.
(prefering left side in this image)

I see, thank you very much for clarifying!

We currently have a recommendation that distutils should be imported after setuptools expressed in

setuptools/_distutils_hack/__init__.py

Lines 17 to 24 in a6f7484

warnings.warn(

"Distutils was imported before Setuptools, but importing Setuptools "

"also replaces the `distutils` module in `sys.modules`. This may lead "

"to undesirable behaviors or errors. To avoid these issues, avoid "

"using distutils directly, ensure that setuptools is installed in the "

"traditional way (e.g. not an editable install), and/or make sure "

"that setuptools is always imported before distutils."

)

So I think that honouring that is more important than the traditional approach.

I understand that indirectly we can verify that both approaches work. But the implicit approach might be fragile in the future¹. So in principle, I am in favour of directly ensuring that any import of distutils always come explicitly after the imports to setuptools².

Footnotes

For example to solve cycles, Python import machinery may start evaluating the parent module, then interrupt this evaluation in favour of children module, then proceed to finish the evaluation of the parent module. ↩

Unless we verify an unintended consequence and we need to revert. ↩

We can change that once SETUPTOOLS_USE_DISTUTILS=stdlib is fully removed.

I'll follow your decision! I think my case was clear and understood so I'm satisfied. I've rebased the config and autofix changes separately to keep that nice 2 commits separation.

Avasam · 2024-08-12T12:20:55Z

pkg_resources/__init__.py

@@ -93,20 +91,16 @@
    # no write support, probably under GAE


Question that doesn't affect this PR: What's GAE ?

Google App Engine? I don't know... the change is from 2009, before setuptools moved to Github and unfortunatelly all references to issues/PR have been messed up.

That tracks. Ref https://code.google.com/archive/p/appengine-monkey/issues/25 / quankim277/appengine-monkey#25

…hack

abravalheri · 2024-08-12T15:29:41Z

Thank you very much.

This might add some extra work for the currently opened PRs, but I think it is good in the long term.

Avasam · 2024-08-12T16:40:55Z

some extra work for the currently opened PRs

The good news is it shouldn't be much: accept both on merge conflict concerning imports, then let Ruff do the rest.

Avasam force-pushed the ruff-isort branch 2 times, most recently from 913eedb to 6287b8d Compare June 14, 2024 20:29

Avasam force-pushed the ruff-isort branch 2 times, most recently from 2f2f68a to d8c8dcc Compare July 1, 2024 16:17

Avasam changed the title ~~Sort imports for more consistent diff~~ Sort imports for more consistent diff (ruff/isort) Jul 1, 2024

Avasam commented Jul 1, 2024

View reviewed changes

ruff.toml Outdated Show resolved Hide resolved

Avasam mentioned this pull request Jul 20, 2024

Enforce return types for typed public functions & mark pkg_resources as py.typed #4409

Merged

2 tasks

Avasam commented Jul 20, 2024

View reviewed changes

Avasam force-pushed the ruff-isort branch from c92ae1f to c2becc5 Compare July 20, 2024 18:48

Avasam commented Jul 20, 2024

View reviewed changes

Avasam force-pushed the ruff-isort branch from b80753d to 7d0b00c Compare July 30, 2024 15:35

abravalheri reviewed Aug 9, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

Avasam force-pushed the ruff-isort branch from 513b03e to 8bf7234 Compare August 9, 2024 19:22

abravalheri requested changes Aug 12, 2024

View reviewed changes

Avasam commented Aug 12, 2024

View reviewed changes

Avasam added 2 commits August 12, 2024 10:53

Configure Ruff/Isort for consistent imports accounting for distutils …

5349827

…hack

Apply Ruff/Isort

9081bcf

Avasam force-pushed the ruff-isort branch from 8bf7234 to 9081bcf Compare August 12, 2024 14:53

Avasam requested a review from abravalheri August 12, 2024 15:00

abravalheri approved these changes Aug 12, 2024

View reviewed changes

abravalheri merged commit fa10e36 into pypa:main Aug 12, 2024
21 checks passed

Avasam deleted the ruff-isort branch August 12, 2024 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort imports for more consistent diff (ruff/isort) #4392

Sort imports for more consistent diff (ruff/isort) #4392

Avasam commented May 23, 2024 •

edited

Loading

Avasam Jul 20, 2024

Avasam Jul 20, 2024

Avasam Jul 20, 2024

Avasam Jul 20, 2024

abravalheri left a comment •

edited

Loading

abravalheri Aug 9, 2024 •

edited

Loading

Avasam Aug 9, 2024

Avasam commented Aug 9, 2024 •

edited

Loading

abravalheri left a comment

abravalheri Aug 12, 2024 •

edited

Loading

Avasam Aug 12, 2024 •

edited

Loading

abravalheri Aug 12, 2024 •

edited

Loading

Avasam Aug 12, 2024 •

edited

Loading

abravalheri Aug 12, 2024

abravalheri Aug 12, 2024

Avasam Aug 12, 2024

Avasam Aug 12, 2024

abravalheri Aug 12, 2024 •

edited

Loading

Avasam Aug 12, 2024 •

edited

Loading

abravalheri commented Aug 12, 2024

Avasam commented Aug 12, 2024

		from os import open as os_open, utime # isort: skip
		from os.path import isdir, split # isort: skip

		known-third-party = ["setuptools"]
		known-first-party = ["distutils"]

-known-third-party = ["setuptools"]
-known-first-party = ["distutils"]
+section-order = ["future", "standard-library", "eager", "third-party", "first-party", "local-folder", "delayed"]
+sections.eager = ["_distutils_hack"]
+sections.delayed = ["distutils"]

	section-order = ["future", "standard-library", "eager", "third-party", "first-party", "delayed", "local-folder"]
	section-order = ["future", "standard-library", "eager", "third-party", "first-party", "local-folder", "delayed"]

	warnings.warn(
	"Distutils was imported before Setuptools, but importing Setuptools "
	"also replaces the `distutils` module in `sys.modules`. This may lead "
	"to undesirable behaviors or errors. To avoid these issues, avoid "
	"using distutils directly, ensure that setuptools is installed in the "
	"traditional way (e.g. not an editable install), and/or make sure "
	"that setuptools is always imported before distutils."
	)

Sort imports for more consistent diff (ruff/isort) #4392

Sort imports for more consistent diff (ruff/isort) #4392

Conversation

Avasam commented May 23, 2024 • edited Loading

Summary of changes

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri left a comment • edited Loading

Choose a reason for hiding this comment

abravalheri Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avasam commented Aug 9, 2024 • edited Loading

abravalheri left a comment

Choose a reason for hiding this comment

abravalheri Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Avasam Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

abravalheri Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Avasam Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Avasam Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

abravalheri commented Aug 12, 2024

Avasam commented Aug 12, 2024

Avasam commented May 23, 2024 •

edited

Loading

abravalheri left a comment •

edited

Loading

abravalheri Aug 9, 2024 •

edited

Loading

Avasam commented Aug 9, 2024 •

edited

Loading

abravalheri Aug 12, 2024 •

edited

Loading

Avasam Aug 12, 2024 •

edited

Loading

abravalheri Aug 12, 2024 •

edited

Loading

Avasam Aug 12, 2024 •

edited

Loading

abravalheri Aug 12, 2024 •

edited

Loading

Avasam Aug 12, 2024 •

edited

Loading