-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fail fast if site-packages are in the path #5301
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
Conversation
mypy/build.py
Outdated
package_path = tuple(_get_site_packages_dirs(options.python_executable)) | ||
for site in package_path: | ||
if site in mypypath: | ||
print("site-packages is in the MYPYPATH. Please remove it.", file=sys.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there may be multiple site-packages dirs (else why the outer loop :-) it would be helpful to include the offending site value in the messages.
mypy/build.py
Outdated
print("site-packages is in the PYTHONPAT. Please change directory so it is not.", | ||
file=sys.stderr) | ||
elif site in lib_path: | ||
print("this should never, ever happen", file=sys.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a capital letter and a trailing period. Also why should this never happen? And if if can't happen perhaps it should be an assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should never happen because the path to the typeshed stubs should never include a site-package directory. I will change it to an assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being so nitpicky.
mypy/build.py
Outdated
else: | ||
# this site not in any of the other paths, we are safe. | ||
continue | ||
sys.exit(1) | ||
return SearchPaths(tuple(reversed(python_path)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel there ought to be a blank line here (like you had before the last commit).
mypy/build.py
Outdated
else: | ||
# this site not in any of the other paths, we are safe. | ||
continue | ||
sys.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it's better to have two sys.exit(1)
calls, one after each print()
. Then you don't need to else: continue
bit which feels a bit odd.
package_path = tuple(_get_site_packages_dirs(options.python_executable)) | ||
for site_dir in package_path: | ||
assert site_dir not in lib_path | ||
if site_dir in mypypath: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are of course many variant spellings of the same path that won't be caught by this check. I don't think it's necessary to worry about that (normalizing the paths feels kind of expensive), but we shouldn't feel too complacent. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it won't be perfect, but I think it will catch and help debug some more common cases, as well as avoid crashes in those circumstances.
Not at all, code quality matters. I appreciate constructive feedback, no matter how small :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG now!
Looks to be related to this pull request, if I run:
I get
And if I run
I get
In one word he didn't find anything ... Did I something wrong? |
Try |
If I add that It pass but it nearly didn't check anything... |
Hi, unfortunately, unless a package is marked as type safe, we do not inspect it. You may want to read about this in the docs: https://mypy.readthedocs.io/en/latest/installed_packages.html. If you have further questions, feel free to drop by our Gitter chat. |
OK thanks @ethanhs :-) |
With my refactoring in #5256, I realized that it would be pretty straightforward to give some nicer warnings if site-packages were in any of the PATHs.