Skip to content

Nested namespace packages support #10541

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 25, 2024
Merged

Nested namespace packages support #10541

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

@github-actions GitHub Actions
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
@dizzy57 dizzy57 deleted the nested-namespace-packages branch March 27, 2024 18:41
@BrewTestBot BrewTestBot mentioned this pull request Apr 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

@charliermarsh charliermarsh

Labels
isort Related to import sorting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dizzy57 @charliermarsh