modulefinder: make -p handle namespace packages correctly #9616
Conversation
This file contains 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
added 3 commits
October 19, 2020 01:10
Fixes part of python#5759 The other part is passing files arguments, which we make steps towards in python#9614
JukkaL
reviewed
Oct 23, 2020
mypy/modulefinder.py
Outdated
| @@ -387,13 +387,13 @@ def find_modules_recursive(self, module: str) -> List[BuildSource]: | |||
| if mod not in hits: | |||
| hits.add(mod) | |||
| result += self.find_modules_recursive(module + '.' + mod) | |||
| elif os.path.isdir(module_path) and module in self.ns_packages: | |||
| # Even more subtler: handle recursive decent into PEP 420 | |||
| elif os.path.isdir(module_path) and any(module.startswith(p) for p in self.ns_packages): | |||
There was a problem hiding this comment.
I wonder if startswith is correct here. For example, consider foobar vs foo?
There was a problem hiding this comment.
Good point, but now that I'm looking at the check, I think it's unnecessary (always true, and running tests with an assert seems to confirm that).
JukkaL
approved these changes
Oct 24, 2020
This was referenced Oct 29, 2020
hauntsaninja
pushed a commit
to hauntsaninja/mypy
that referenced
this pull request
Oct 31, 2020
This was removed in python#9616
hauntsaninja
added a commit
that referenced
this pull request
Dec 12, 2020
This is the successor to #9632. Things should basically be as discussed in that PR. Since #9616 is merged, this should now resolve #5759. We leave the Bazel integration with `--package-root` almost entirely untouched, save for a) one change that's a bugfix / doesn't affect the core of what `--package-root` is doing, b) another drive by bugfix that's not related to this PR. Change a) fixes the package root `__init__.py` hackery when passed absolute paths. Change b) fixes the validation logic for package roots above the current directory; it was broken if you passed `..` as a package root Since we're leaving `--package-root` alone for now, I named the new flag `--explicit-package-base` to try and avoid confusion. Doing so also matches the language used by BuildSource a little better. The new logic is summarised in the docstring of `SourceFinder.crawl_up`. Some commentary: - I change `find_sources_in_dir ` to call `crawl_up` directly to construct the BuildSource. This helps codify the fact that files discovered will use the same module names as if you passed them directly. - Doing so keeps things DRY with the more complicated logic and means, for instance, that we now do more sensible things in some cases when we recursively explore directories that have invalid package names. - Speaking of invalid package names, if we encounter a directory name with an invalid package name, we stop crawling. This is necessary because with namespace packages there's no guarantee that what we're crawling was meant to be a Python package. I add back in a check in the presence of `__init__.py` to preserve current unit tests where we raise InvalidSourceList. - The changes to modulefinder are purely cosmetic and can be ignored (there's some similar logic between the two files and this just makes sure they mirror each other closely) - One notable change is we now always use absolute paths to crawl. This makes the behaviour more predictable and addresses a common complaint: fixes #9677, fixes #8726 and others. - I figured this could use more extensive testing than a couple slow cmdline tests. Hopefully this test setup also helps clarify the behaviour :-) Co-authored-by: hauntsaninja <>
hauntsaninja
pushed a commit
to hauntsaninja/mypy
that referenced
this pull request
Jan 18, 2021
This should cover the current state on master, as implemented across python#9742, python#9683, python#9632, python#9616, python#9614, etc. This will need to be changed if we can make `--namespace-packages` the default (python#9636).
hauntsaninja
pushed a commit
to hauntsaninja/mypy
that referenced
this pull request
Jan 18, 2021
This should cover the current state on master, as previously discussed / implemented across python#9742, python#9683, python#9632, python#9616, python#9614, etc. This will need to be changed if we can make `--namespace-packages` the default (python#9636). I haven't documented some of the finer points of the changes, since it felt like an inappropriate level of detail (e.g. using absolute paths when crawling, how directories with invalid package names affect crawling, etc)
hauntsaninja
pushed a commit
to hauntsaninja/mypy
that referenced
this pull request
Jan 18, 2021
This should cover the current state on master, as previously discussed / implemented across python#9742, python#9683, python#9632, python#9616, python#9614, etc. This will need to be changed if we can make `--namespace-packages` the default (python#9636). I haven't documented some of the finer points of the changes, since it felt like an inappropriate level of detail (e.g. using absolute paths when crawling, how directories with invalid package names affect crawling, etc)
hauntsaninja
added a commit
that referenced
this pull request
Jan 19, 2021
This should cover the current state on master, as previously discussed / implemented across #9742, #9683, #9632, #9616, #9614, etc. This will need to be changed if we can make `--namespace-packages` the default (#9636). I haven't documented some of the finer points of the changes, since it felt like an inappropriate level of detail (e.g. using absolute paths when crawling, how directories with invalid package names affect crawling, etc) Co-authored-by: hauntsaninja <>
ilevkivskyi
pushed a commit
that referenced
this pull request
Jan 20, 2021
This should cover the current state on master, as previously discussed / implemented across #9742, #9683, #9632, #9616, #9614, etc. This will need to be changed if we can make `--namespace-packages` the default (#9636). I haven't documented some of the finer points of the changes, since it felt like an inappropriate level of detail (e.g. using absolute paths when crawling, how directories with invalid package names affect crawling, etc) Co-authored-by: hauntsaninja <>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes part of #5759
The other part is passing files arguments, which we make steps towards
in #9614