[isort] Check full module path against project root(s) when categorizing first-party#16565
Merged
dylwil3 merged 13 commits intoastral-sh:mainfrom May 5, 2025
Merged
[isort] Check full module path against project root(s) when categorizing first-party#16565dylwil3 merged 13 commits intoastral-sh:mainfrom
isort] Check full module path against project root(s) when categorizing first-party#16565dylwil3 merged 13 commits intoastral-sh:mainfrom
Conversation
dylwil3
added
bug
Contributor
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| I001 | 35 | 22 | 13 | 0 | 0 |
Formatter (stable)
ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)
mesonbuild/meson-python (error)
warning: Detected debug build without --no-cache.
error: Failed to read tests/packages/symlinks/baz.py: No such file or directory (os error 2)
error: Failed to read tests/packages/symlinks/qux.py: No such file or directory (os error 2)
Formatter (preview)
ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)
mesonbuild/meson-python (error)
ruff format --preview
warning: Detected debug build without --no-cache.
error: Failed to read tests/packages/symlinks/baz.py: No such file or directory (os error 2)
error: Failed to read tests/packages/symlinks/qux.py: No such file or directory (os error 2)
5cb8c06 to
2eb5cd5
Compare
dylwil3
changed the title
isort] Only infer subpackages of namespace packages as first-partyisort] Check full module path against project root(s) when categorizing first-party
Member
|
I think that PR slipped our attention last week. At least I didn't notice it :) |
Member
|
Did you click through the ecosystem checks? I'm torn but I think this should be gated behind preview. The ecosystem fallouts are significant enough. |
Member
|
I haven't forgotten about this. I'm still waging war against my notification queue after the on-site 😢 |
Collaborator
Author
No worries!
I did quickly before, but going through them again I noticed two examples that need more investigation.
Good point - I'll try to sort out the least ugly way to do that... Finally, I think I should try to fix the following limitation of the current approach. When we see from foo import barthe former approach would look for a from foo import baror import foo.bar as barI'm going to revert back to draft while I sort out all of the above. Sorry for the needless ping! |
dylwil3
force-pushed
the
namespaces
branch
2 times, most recently
from
ca727da to
7ea88ab
Compare
Member
|
I haven't looked at the code yet but could you update the PR summary or add a new comment that replies to your ecosystem findings and explains why we think they're correct. This will help us to remember the reasoning when writing the changelog in the next minor release |
MichaReiser
reviewed
Apr 23, 2025
MichaReiser
approved these changes
Apr 23, 2025
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thanks to work on this. This looks good to me. I've a few smaller nit comments and I think we need to extend the documentation and the motivation for the change a bit before landing.
crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs
Show resolved
Hide resolved
Collaborator
Author
|
Note: There have been some other issues raised in #10519 but I think they are orthogonal to this fix (the issues are present both before and after the current change) so it's worth exploring solutions in a followup. |
Member
|
I basically lost all context on this because the work I did on it was such a long time ago :-( if @MichaReiser is happy with this, I'm happy with it! I trust both of you to get it right 😃 |
AlexWaygood
pushed a commit
that referenced
this pull request
May 5, 2025
…izing first-party (#16565) When attempting to determine whether `import foo.bar.baz` is a known first-party import relative to [user-provided source paths](https://docs.astral.sh/ruff/settings/#src), when `preview` is enabled we now check that `SRC/foo/bar/baz` is a directory or `SRC/foo/bar/baz.py` or `SRC/foo/bar/baz.pyi` exist. Previously, we just checked the analogous thing for `SRC/foo`, but this can be misleading in situations with disjoint namespace packages that share a common base name (e.g. we may be working inside the namespace package `foo.buzz` and importing `foo.bar` from elsewhere). Supersedes #12987 Closes #12984
Glyphack
pushed a commit
to Glyphack/ruff
that referenced
this pull request
May 6, 2025
…izing first-party (astral-sh#16565) When attempting to determine whether `import foo.bar.baz` is a known first-party import relative to [user-provided source paths](https://docs.astral.sh/ruff/settings/#src), when `preview` is enabled we now check that `SRC/foo/bar/baz` is a directory or `SRC/foo/bar/baz.py` or `SRC/foo/bar/baz.pyi` exist. Previously, we just checked the analogous thing for `SRC/foo`, but this can be misleading in situations with disjoint namespace packages that share a common base name (e.g. we may be working inside the namespace package `foo.buzz` and importing `foo.bar` from elsewhere). Supersedes astral-sh#12987 Closes astral-sh#12984
dylwil3
added a commit
that referenced
this pull request
Sep 5, 2025
This stabilizes the behavior introduced in #16565 which (roughly) tries to match an import like `import a.b.c` to an actual directory path `a/b/c` in order to label it as first-party, rather than simply looking for a directory `a`. Mainly this affects the sorting of imports in the presence of namespace packages, but a few other rules are affected as well.
ntBre
pushed a commit
that referenced
this pull request
Sep 8, 2025
This stabilizes the behavior introduced in #16565 which (roughly) tries to match an import like `import a.b.c` to an actual directory path `a/b/c` in order to label it as first-party, rather than simply looking for a directory `a`. Mainly this affects the sorting of imports in the presence of namespace packages, but a few other rules are affected as well.
ntBre
pushed a commit
that referenced
this pull request
Sep 10, 2025
This stabilizes the behavior introduced in #16565 which (roughly) tries to match an import like `import a.b.c` to an actual directory path `a/b/c` in order to label it as first-party, rather than simply looking for a directory `a`. Mainly this affects the sorting of imports in the presence of namespace packages, but a few other rules are affected as well.
ntBre
pushed a commit
that referenced
this pull request
Sep 10, 2025
This stabilizes the behavior introduced in #16565 which (roughly) tries to match an import like `import a.b.c` to an actual directory path `a/b/c` in order to label it as first-party, rather than simply looking for a directory `a`. Mainly this affects the sorting of imports in the presence of namespace packages, but a few other rules are affected as well.
ntBre
pushed a commit
that referenced
this pull request
Sep 10, 2025
This stabilizes the behavior introduced in #16565 which (roughly) tries to match an import like `import a.b.c` to an actual directory path `a/b/c` in order to label it as first-party, rather than simply looking for a directory `a`. Mainly this affects the sorting of imports in the presence of namespace packages, but a few other rules are affected as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When attempting to determine whether
import foo.bar.bazis a known first-party import relative to user-provided source paths, whenpreviewis enabled we now check thatSRC/foo/bar/bazis a directory orSRC/foo/bar/baz.pyorSRC/foo/bar/baz.pyiexist.Previously, we just checked the analogous thing for
SRC/foo, but this can be misleading in situations with disjoint namespace packages that share a common base name (e.g. we may be working inside the namespace packagefoo.buzzand importingfoo.barfrom elsewhere).Supersedes #12987
Closes #12984
Testing
We've added some unit tests displaying the different behavior in the function
match_sourceswhich implements this logic. In addition, here are the results of an ad-hoc, local test and comparison withisort. Notice that the preview behavior now coincides withisort- including the inconsistent classification offromimports.(I've manually edited out some of the irrelevant logs and such in the outputs)
Summary of Ecosystem Checks
snowcliThese are all true positives! The package in this repo is
snowflake.cli, and the ecosystem reports all have to do with importing third-party packages likesnowflake.snowparkandsnowflake.connector.airflowAlso true positives: the dependency
docs.utils.conf_constantsappears in various places and is correctly identified as third-party, whereas before it was considered first-party because adocsfolder.freedomofpressThis is a false positive because this repo contains a Python file that does not have a
.pysuffix, but instead uses the shebang specification: https://github.com/freedomofpress/securedrop/blob/develop/journalist_gui/SecureDropUpdatermlflowThe change is a true positive, but there is a leftover false positive. The change concerns two imports:
On
main, these are both incorrectly identified as first-party due to a local folder nameddocker. In this PR with preview enabled,docker.errorsis correctly identified as third-party, which causes a lint error to be emitted.pytestThe problematic import here is
from py.path import errorwhich is, unfortunately, incorrectly labeled as third-party inpreview. This is because it is not possible to know this import is first party from the file system alone! Indeed,pathis actually a submodule of a module called_py, and then we have:If that's not bad enough, if you go to
src/_pytest/_pyyou will not findpath/errorinstead you will findpath.pywhich has this line:I don't think it's in scope to support something like that here! Maybe one day
tywill power our import sorting!