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

Allow multiple shadow files to be specified #5023

Merged
merged 10 commits into from May 16, 2018

Conversation

Projects
None yet
3 participants
@garetht
Contributor

garetht commented May 12, 2018

This pull request allows the --shadow-file argument to be specified multiple times, allowing multiple different source files to be replaced by their corresponding shadow files.

Tests have also been added to ensure the original and new behaviors both work.

@garetht

This comment has been minimized.

Contributor

garetht commented May 14, 2018

Not sure why the tests are failing only in Python 3.5.1 - it seems the newly added method hasn't made its way into that test?

@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented May 14, 2018

I think you'll have to type: ignore it until python/typeshed#2053 is merged. The issue appears only in 3.5.1 probably because we run additional tests on 3.5.1 only (see .travis.yml).

@gvanrossum

Good work! I have some suggestions for tidying up the code a bit, and some requests to clarify the docs (since this feature has been widely misunderstood).

@@ -468,7 +468,8 @@ Here are some more useful flags:
make transformations to a file before type checking without having to change
the file in-place. (For example, tooling could use this to display the type
of an expression by wrapping it with a call to reveal_type in the shadow
file and then parsing the output.)
file and then parsing the output.) This argument may be specified multiple times
to make mypy substitute multiple different files with their shadow replacements.

This comment has been minimized.

@gvanrossum

gvanrossum May 15, 2018

Member

This needs an example -- do you write --shadow-file X1 X2 Y1 Y2 or --shadow-file X1 X2 --shadow-file Y1 Y2? (I presume the latter, but some options use the former syntax, and argparse supports both.)

Also, this argument has caused many people to be confused. We should present an example explaining that --shadow-file X1 X2 means that whenever the checker is asked to check X1, it actually reads and checks the contents of X2, but diagnostics will refer to X1 (it may well be the latter bit that trips people up).

if (self.options.shadow_file and
os.path.samefile(self.options.shadow_file[0], path)):
path = self.options.shadow_file[1]
if not self.options.shadow_file:

This comment has been minimized.

@gvanrossum

gvanrossum May 15, 2018

Member

IMO it would be better if this checked for if not self.shadow_map. The Options object has been processed already in __init__() above.

else:
self.shadow_equivalence_map[path] = None
shadow_file = self.shadow_equivalence_map.get(path)

This comment has been minimized.

@gvanrossum

gvanrossum May 15, 2018

Member

This and the rest of the function could be shortened to

return self.shadow_equivalence_map.get(path, path)
previously_checked = path in self.shadow_equivalence_map
if not previously_checked:
for k in self.shadow_map.keys():

This comment has been minimized.

@gvanrossum

gvanrossum May 15, 2018

Member

I'd switch this to for k, v in self.shadow_map.items(), then you can use v instead of self.shadow_map.get(k) two lines below.

(Also perhaps use more imaginative names than k, v -- how about source, shadow?

def samefile(self, f1: str, f2: str) -> bool:
s1 = self.stat(f1)
s2 = self.stat(f2)
return os.path.samestat(s1, s2) # type: ignore

This comment has been minimized.

@gvanrossum

gvanrossum May 15, 2018

Member

I'm curious -- why do you need # type: ignore here?

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 15, 2018

Collaborator

We were missing this function in the stub. We just merged the PR adding it, so this ignore can be removed as soon as we sync typeshed.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented on mypy/fscache.py in 605a37d May 15, 2018

Looks like python/typeshed#2053 was actually merged so you can get rid of this again.

garetht and others added some commits May 15, 2018

Add extra space to help message
Avoid run-in words due to line break. (Also switched to "Human quotes".)

@gvanrossum gvanrossum merged commit 2a0c4e5 into python:master May 16, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gvanrossum

This comment has been minimized.

Member

gvanrossum commented May 16, 2018

Thanks! (I cancelled the tests of my final cleanup since Travis and AppVeyor are kind of busy.)

@garetht

This comment has been minimized.

Contributor

garetht commented May 16, 2018

Thanks for the quick review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment