Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested namespace packages support #10541

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

dizzy57
Copy link
Contributor

@dizzy57 dizzy57 commented Mar 24, 2024

Summary

PEP 420 says nested namespace packages are allowed, i.e. marking a directory as a namespace package marks all subdirectories in the subtree as namespace packages.

is_package is modified to use Path::starts_with and the order of checks is reversed to do in-memory checks first before hitting the disk.

Test Plan

Added unit tests. Previously all tests were run with namespace_packages == &[]. Verified that one of the tests was failing before changing the implementation.

Future Improvements

The is_package_with_cache can probably be rewritten to avoid repeated calls to Path::starts_with, by caching all directories up to the namespace_root:

let namespace_root = namespace_packages
    .iter()
    .filter(|namespace_package| path.starts_with(namespace_package))
    .min();

@dizzy57
Copy link
Contributor Author

dizzy57 commented Mar 24, 2024

Related: #6114 #5149

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh self-requested a review March 24, 2024 20:07
@charliermarsh charliermarsh self-assigned this Mar 24, 2024
@charliermarsh
Copy link
Member

Did you run into an issue with this in practice that motivated this change?

@dizzy57
Copy link
Contributor Author

dizzy57 commented Mar 24, 2024

Did you run into an issue with this in practice that motivated this change?

We have a monorepo where all the files are located in an organization/project/subproject/ structure and both organization/ and organization/project/ directories are meant to be namespace packages, so no __init__.py in them.

We wanted to use banned-api (TID251), but for python sources it detected subproject as python package name. We didn't want to mark all the organization/project/ directories as namespace packages (it would be brittle, as more projects might be added without changing the ruff config), but marking just organization/ as a namespace package didn't work.

@dizzy57
Copy link
Contributor Author

dizzy57 commented Mar 24, 2024

This test illustrates our situation pretty well. Currently ruff assigns package name core to this directory, but we want it to be recognized as python_modules.core.core with only python_modules being marked as namespace package.

https://github.com/dizzy57/ruff/blob/7607b2d64083c797aa334b3f2e767a2a12bf191b/crates/ruff_linter/src/packaging.rs#L111-L117

@charliermarsh
Copy link
Member

Okay thanks. I think this is correct.

@charliermarsh charliermarsh added the isort Related to import sorting label Mar 25, 2024
@charliermarsh charliermarsh merged commit d625f55 into astral-sh:main Mar 25, 2024
17 checks passed
@dizzy57 dizzy57 deleted the nested-namespace-packages branch March 27, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
isort Related to import sorting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants