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

Silence modules in site-packages and typeshed #5303

Merged
merged 7 commits into from
Jul 6, 2018

Conversation

emmatyping
Copy link
Collaborator

Fixes #5260 .

@emmatyping
Copy link
Collaborator Author

emmatyping commented Jul 1, 2018

Huh, this is strange:

from typing import Sequence
from os import fspath
s: Sequence[str]

s = tuple(map(fspath, s))
reveal_type(s)  # error: Revealed type is '<nothing>'

I suppose the map messes things up.

@ilevkivskyi
Copy link
Member

@ethanhs Is this a regression or it was always like this? (There is a chance this is related to a recent overload overhaul.)

@emmatyping
Copy link
Collaborator Author

Is this a regression or it was always like this?

Not sure, I created the repro with https://mypy-play.net, which uses 0.610, I haven't tested it on older versions.

@ilevkivskyi
Copy link
Member

No, it is a meet.is_overlapping_types problem which is still quite simplistic, it considers typing.Sequence[builtins.str] and builtins.tuple[builtins.str] non-overlapping (surprise).

The fix should be straightforward, but if you don't have time, just open an issue about this.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

The code looks good, but I think we should add a test that installs a PEP 561 with various errors (both import related and non-import-related) to be sure they are all silenced.

@@ -751,15 +751,16 @@ main:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports"
main:2: error: Incompatible types in assignment (expression has type "int", variable has type "str")

[case testAddFileWhichImportsLibModuleWithErrors]
# flags: --no-silence-site-packages
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary here? Is broken installed as a PEP 561 package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since opening this, I also implemented silencing typeshed, which broken is simulated as being part of via alt_lib_path.

mypy/util.py Outdated

# backport commonpath for 3.4
if sys.platform == 'win32':
def commonpath(paths: Sequence[str]) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the general algorithm here? As I understand we essentially only need something like is_sub_path(p1, p2). Not that I think it is important, but maybe it will allow to simplify code?

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 was originally hoping to eventually use os.path.commonpath once we drop support for 3.4, but I think you are right it would be better to do this ourselves. I will change this when I have time later.

@emmatyping
Copy link
Collaborator Author

I think we should add a test that installs a PEP 561 with various errors

I will add some errors to typedpkg to test the functionality.

@emmatyping emmatyping changed the title Silence modules in site-packages Silence modules in site-packages and typeshed Jul 5, 2018
@ilevkivskyi
Copy link
Member

I will add some errors to typedpkg to test the functionality.

OK, thanks! Just note that I would like to cut the release branch tomorrow morning. So it would be better to have the tests by that time.

@emmatyping
Copy link
Collaborator Author

Just note that I would like to cut the release branch tomorrow morning. So it would be better to have the tests by that time.

Sure thing, I will do that momentarily.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

OK, looks good now!

@ilevkivskyi ilevkivskyi merged commit 1e7b517 into python:master Jul 6, 2018
@emmatyping emmatyping deleted the silencepep561 branch July 22, 2018 02:53
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.

2 participants