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

feat: add back PEP 420 support behind feature flag #808

Merged
merged 5 commits into from
Aug 10, 2024

Conversation

mkniewallner
Copy link
Collaborator

@mkniewallner mkniewallner commented Aug 10, 2024

Relates to #740.

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added or modified a feature, documentation in docs is updated

Description of changes

Add back support for namespace package (PEP 420) that was reverted in #798 because of a performance degradation on large codebases.

2 changes in this new implementation:

  • it is guarded by an experimental flag (--experimental-namespace-package), so it should not be considered stable (but people that relied on this new feature will at least be able to opt back in)
  • we use os.walk to stop on the first Python file we found, rather than using the expensive Path.rglob, which will loop through all directories

The performance degradation could still probably be noticeable on projects that are prone to have a directory with a huge amount of nested non-Python directories (for instance node_modules).

As possible future improvements:

  • we could probably lazily check if a module is a local one when discovering imports -- that way, we only run _is_local_module if we need to do so, avoiding irrelevant directories that don't match any known import
  • we could delegate file discovery to Rust, which should be more performant

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.0%. Comparing base (b5a5478) to head (a8eea2f).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #808   +/-   ##
=====================================
  Coverage   93.0%   93.0%           
=====================================
  Files         37      37           
  Lines        961     971   +10     
  Branches     173     177    +4     
=====================================
+ Hits         894     904   +10     
  Misses        51      51           
  Partials      16      16           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mkniewallner mkniewallner force-pushed the feat/add-back-pep-420-behind-flag branch from 3c443b0 to fac0aef Compare August 10, 2024 11:51
@mkniewallner mkniewallner force-pushed the feat/add-back-pep-420-behind-flag branch from fac0aef to a8eea2f Compare August 10, 2024 12:09
@mkniewallner mkniewallner marked this pull request as ready for review August 10, 2024 12:36
@mkniewallner mkniewallner merged commit 6d2344b into main Aug 10, 2024
25 checks passed
@mkniewallner mkniewallner deleted the feat/add-back-pep-420-behind-flag branch August 10, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant