NamedTemporaryFile works different on Windows #26
Conversation
Add optional stream parameters to generate_annotation_json and parse_json
There was a problem hiding this comment.
Please run mypy --ignore-missing-imports --strict-optional (as well as pytest) before pushing your next commit.
I also wonder if perhaps it wouldn't make more sense to pass the dicts around rather than going through a temporary file. (I only did it that way because Jukka had written his half of the code to write to a file exclusively.)
.gitignore
Outdated
| @@ -2,3 +2,5 @@ | |||
| /*.egg-info | |||
| /build | |||
| /dist | |||
| .idea/ | |||
| *.pyc | |||
There was a problem hiding this comment.
These two (but especially .idea) belong in your own global .gitignore (which you have to configure using something like
[core]
excludesfile = /Users/guido/.gitignore_global
in your ~/.gitconfig.
There was a problem hiding this comment.
I'll take them out.
I've gotten in the habit at work of not presuming that people's global .gitignores are correct.
| @@ -42,7 +42,7 @@ def main(): | |||
|
|
|||
| # Run pass 2 with output written to a temporary file. | |||
| infile = args.type_info | |||
| generate_annotations_json(infile, tf.name) | |||
| generate_annotations_json(infile, tf.name, target_stream=tf.file) | |||
There was a problem hiding this comment.
You don't need to use the .file attribute -- just pass tf, it's a file alright.
pyannotate_tools/annotations/main.py
Outdated
| @@ -1,8 +1,9 @@ | |||
| """Main entry point to mypy annotation inference utility.""" | |||
|
|
|||
| import json | |||
| from io import IOBase | |||
There was a problem hiding this comment.
I wouldn't do that. I'd import IO from typing and use IO[str] as the type of the streams everywhere.
pyannotate_tools/annotations/main.py
Outdated
| def generate_annotations_json(source_path, target_path): | ||
| # type: (str, str) -> None | ||
| def generate_annotations_json(source_path, target_path, source_stream=None, target_stream=None): | ||
| # type: (str, str, Optional[IOBase], Optional[IOBase]) -> None |
| data = json.load(stream) | ||
| else: | ||
| with open(path) as f: | ||
| data = json.load(f) # type: List[RawEntry] |
There was a problem hiding this comment.
The annotation should go on the first assignment to data.
| generate_annotations_json(source_path, target.name) | ||
| target = tempfile.NamedTemporaryFile(mode='w+') | ||
| with self.temporary_json_file(data) as source: | ||
| generate_annotations_json(source[0], target.name, source_stream=source[1], target_stream=target.file) |
There was a problem hiding this comment.
No need for .file here or anywhere.
| @@ -76,7 +78,8 @@ def test_ambiguous_kind(self): | |||
| @contextlib.contextmanager | |||
| def temporary_json_file(self, data): | |||
| # type: (str) -> Iterator[str] | |||
There was a problem hiding this comment.
Update the type here too (Iterator[Tuple[str, IO[str]]]).
|
I've signed the CLA now too |
|
Thanks! And fingers crossed for Windows. (I haven't managed to set up AppVeyor yet. Have you managed to test on Windows manually yet?) |
|
This PR appears to have broken Python 3 - $ pyannotate -w --type-info ./annotations.json $file
Traceback (most recent call last):
File "/Users/rwilliams/src/oss/pyannotate/venv/bin/pyannotate", line 11, in <module>
load_entry_point('pyannotate==1.0.2', 'console_scripts', 'pyannotate')()
File "/Users/rwilliams/src/oss/pyannotate/venv/lib/python3.6/site-packages/pyannotate-1.0.2-py3.6.egg/pyannotate_tools/annotations/__main__.py", line 45, in main
File "/Users/rwilliams/src/oss/pyannotate/venv/lib/python3.6/site-packages/pyannotate-1.0.2-py3.6.egg/pyannotate_tools/annotations/main.py", line 59, in generate_annotations_json
File "/usr/local/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/json/__init__.py", line 180, in dump
fp.write(chunk)
File "/Users/rwilliams/src/oss/pyannotate/venv/lib/python3.6/tempfile.py", line 483, in func_wrapper
return func(*args, **kwargs)
TypeError: a bytes-like object is required, not 'str'We should probably write a test that exercises |
This reverts commit 8363a8b.
|
Ouch. Yeah, we need a test for that! I'm submitting a revert as a PR (#42), but if you make a PR that fixes it (with a test) we can apply that instead. |
|
I'm going to revert it and come up with a way that doesn't require
temporary files at all.
|
Reverts #26 It broke main() on Python 3. I will redo this without silly temp files.
Fixes #26. There are some remaining TODO items: - Add annotations to fixers - Add a test for main and __main__
Fixes #26. Also add more tests.
Add optional stream parameters to generate_annotation_json and parse_json.
Hopefully fixes #25 but unable to run all tests due to lib2to3 quirks on Windows and Ubuntu