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

find_sources: find build sources recursively #9614

Merged
merged 2 commits into from
Oct 23, 2020

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Oct 19, 2020

Fixes #8548

This is important to fix because it's a really common source of
surprising false negatives.

I also lay some groundwork for supporting passing in namespace packages
on the command line. The approach towards namespace packages that this
anticipates is that if you pass in files to mypy and you want mypy to
understand your namespace packages, you'll need to specify explicit
package roots.

We also make many fewer calls to crawl functions, since we just pass
around what we need.

Fixes python#8548

This is important to fix because it's a really common source of
surprising false negatives.

I also lay some groundwork for supporting passing in namespace packages
on the command line. The approach towards namespace packages that this
anticipates is that if you pass in files to mypy and you want mypy to
understand your namespace packages, you'll need to specify explicit
package roots.

We also make many fewer calls to crawl functions, since we just pass
around what we need.
hauntsaninja pushed a commit to hauntsaninja/mypy that referenced this pull request Oct 19, 2020
Fixes part of python#5759
The other part is passing files arguments, which we make steps towards
in python#9614
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this bug! This was pretty bad. Looks good -- my only comments are about additional test cases.

Additional thanks for cleaning up some of the code, even if it wasn't needed for this PR.

@@ -68,6 +81,7 @@ undef
undef
[out]
pkg/a.py:1: error: Name 'undef' is not defined
pkg/subdir/a.py:1: error: Name 'undef' is not defined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test importing from modules found via recursive directory traversal. Test cases where the directory contains __init__.py and where it doesn't exist.

Maybe test this also with --namespace-packages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testCmdlinePackageSlash tests when the directory contains init.py and testCmdlinePackageContainingSubdir tests when it doesn't.
I added importing of the modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There currently isn't an interaction with --namespace-packages and cmdline tests are slow, so I won't do that unless told.
That said, I have another diff coming soon where I flesh out the package root code and make --namespace-packages the default. All these tests pass with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll also update the docs once all these changes (essentially important bug fixes + first three points of #8584) are shipped :-)

@hauntsaninja hauntsaninja merged commit 0fb671c into python:master Oct 23, 2020
@hauntsaninja hauntsaninja deleted the find branch October 23, 2020 21:56
hauntsaninja added a commit that referenced this pull request Oct 24, 2020
Fixes part of #5759
The other part is passing files arguments, which we make steps towards
in #9614

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 <>
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Jan 25, 2021
With python/mypy#9614, mypy now tries to collect
all files in tests/, but it fails due to:

    tests/end2end/conftest.py: error: Duplicate module named 'conftest' (also at 'tests/end2end/features/conftest.py')
    tests/end2end/conftest.py: error: Are you missing an __init__.py?  [misc]

We should probably add __init__.py files to tests/ at some point...

See #6059 and #5249
@intgr
Copy link
Contributor

intgr commented Jan 27, 2021

Ugh, turns out many users were relying on the old behavior and this causes issues because mypy now finds unexpected Python files from node_modules, virtualenv paths (venv), temporary directories etc. And there isn't a good way to exclude those paths (feature request #4675).

For example, creating an empty virtualenv with Python 3.9 breaks mypy:

% mkdir test
% cd test
% python3 -m venv ./venv
% echo 'a: str = 10' > test.py
% mypy .
venv/lib/python3.9/site-packages/setuptools/_distutils/command/bdist_msi.py:355: error: expected an indented block
Found 1 error in 1 file (errors prevented further checking)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think this is a big enough regression that this change should be reverted in a hotfix release, until a longer-term solution can be figured out.

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.

mypy doesn't recurse through directories, but -h says it does
3 participants