Allow multiple shadow files to be specified#5023
Conversation
|
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? |
|
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
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
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).
docs/source/command_line.rst
Outdated
| 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. |
There was a problem hiding this comment.
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).
mypy/build.py
Outdated
| 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: |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
This and the rest of the function could be shortened to
return self.shadow_equivalence_map.get(path, path)
mypy/build.py
Outdated
|
|
||
| previously_checked = path in self.shadow_equivalence_map | ||
| if not previously_checked: | ||
| for k in self.shadow_map.keys(): |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
I'm curious -- why do you need # type: ignore here?
There was a problem hiding this comment.
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.
Avoid run-in words due to line break. (Also switched to "Human quotes".)
|
Thanks! (I cancelled the tests of my final cleanup since Travis and AppVeyor are kind of busy.) |
|
Thanks for the quick review! |
3 participants
This pull request allows the
--shadow-fileargument 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.