Add mypy type annotations #23
Conversation
|
Oh, I just remembered that Python 2 doesn't support the type annotation syntax, so they have to be done as comments instead. |
DarwinAwardWinner
force-pushed
the
mypy
branch
2 times, most recently
from
65f4498 to
b1bd9e2
Compare
I had to change the logic a bit in order to make the code type check successfully, mainly just converting a "unicode or bytes" path to unambiguously unicode ASAP.
DarwinAwardWinner
force-pushed
the
mypy
branch
from
e2726b4 to
92641e1
Compare
I only enabled mypy for python 3.3 and above, since I don't think there's a python 2 version. (The python 3 version can check python 2 types, but I'm not sure how to do that within tox.)
DarwinAwardWinner
force-pushed
the
mypy
branch
from
92641e1 to
7534e3a
Compare
|
I dislike that this adds runtime cost :( If type annotations are comments, do we need the imports? |
|
I'm actually not sure if the comments need the imports, since I only use Python 3 these days. I'll look into it and get back to you. |
| _proper_fsync = os.fsync | ||
|
|
||
|
|
||
| if sys.platform != 'win32': | ||
| if hasattr(fcntl, 'F_FULLFSYNC'): | ||
| def _proper_fsync(fd): | ||
| # type: (int) -> None |
There was a problem hiding this comment.
Those are all internal functions, are type annotations really necessary?
| self._path = path | ||
| self._mode = mode | ||
| self._overwrite = overwrite | ||
| self._path = _path_to_unicode(path) # type: Text |
There was a problem hiding this comment.
Paths are whichever type str is in the respective language. Unicode paths break under Python 2 when using non-ASCII characters.
There was a problem hiding this comment.
Also in principle I don't want to force-convert something here just for the type annotations.
There was a problem hiding this comment.
So paths are non-Unicode in Python 2 but Unicode in Python 3?
There was a problem hiding this comment.
Yes I think so. How is open described in MyPy?
| self._mode = mode | ||
| self._overwrite = overwrite | ||
| self._path = _path_to_unicode(path) # type: Text | ||
| self._mode = mode # type: Text |
There was a problem hiding this comment.
Flake8 hates this, please only two spaces of padding.
| ''' | ||
| Open the temporary file. | ||
| ''' | ||
| return self._open(self.get_fileobject) | ||
|
|
||
| @contextlib.contextmanager | ||
| def _open(self, get_fileobject): | ||
| # type: (Callable) -> Iterator[IO] |
There was a problem hiding this comment.
This is a context manager, not a generator/iterator.
There was a problem hiding this comment.
I'm not exactly sure what's going on here, but I think maybe it type-checks the un-decorated function, which, when read as written, is a generator that yields an IO object (i.e. an open file). So this is the return type that mypy accepts.
There was a problem hiding this comment.
mypy is wrong then and doesn't know how to deal with decorators.
|
Regarding the differences between Python 2 and 3, as well as the issue of imposing the runtime cost of loading the typing module, I think there's a good solution to both of these: instead of defining the types inline, I can define them in a stub file in the typeshed repo: https://github.com/python/typeshed#third_party Furthermore, I can provide a different stub file for each version of Python, so the Py2 vs 3, bytes vs unicode issue goes away. How does that sound? Would you be ok with me submitting a PR to typeshed adding types for atomicwrites? |
|
Yes please do that instead (though there appears to be one mayor issue with this repo: python/typeshed#153) |
|
That issue doesn't apply here. It's talking about multiple API-incompatible versions of a module for a single Python version, not the same module for different Python versions. So, I'll go ahead and submit Python 2 & Python 3 stub files for atomicwrites. Thanks for your feedback! |
|
That's the issue I'm talking about too. If I were to release a new version of atomicwrites with API-breakage there would be no way to know if the stubs in typeshed are up to date. |
|
I'll close this, please cc me on the respective PR to typeshed. |
|
I think that's only a problem if you plan to maintain two or more API-incompatible versions of atomicwrites at the same time under the same module name. (Is that even possible on PyPI?) Currently the type annotation stubs are only used by static type checkers such as MyPy. Python itself ignores them completely at runtime. So out-of-date type stubs will not prevent people from using your module. Furthermore, the typeshed project has CI checking all the type stubs with ever commit, so if you update your module in a way that is incompatible with the type stub in typeshed, it should show up in the next CI run and be fixed quickly. |
A total ordering by time is generally not possible. Not every version is immediately deprecated the second the next one comes out. Backports exist, maintenance versions exist.
Fair enough. |
|
My guess is that typeshed is intended to correspond to the latest released version (i.e. whatever pip would install when you don't request a specific version). I can look more into this issue if you want. |
|
Not necessary (it's your freetime!) |
Authorized by atomicwrites author here: untitaker/python-atomicwrites#23 (comment)
|
There still seems to be some confusion regarding what to do about evolution of the atomicwrite package. Until python/typeshed#153 is resolved, typeshed/mypy can only hold a single version of the stubs for a package. @untitaker please let us know whether you're okay with having the stubs in typeshed despite that concern. If you are not, we should just close python/typeshed#705. If you are okay with this, we can go ahead and merge that. But we currently don't have a way to automatically test that stubs match the real library. Alternative you may be able to add type annotations in the form of comments to the atomicwriter package itself. |
|
@gvanrossum This pull request was about adding type comments to the atomicwriter code. @untitaker didn't want to add the typing module as a dependency, which Is why I pursued adding the annotations to the typeshed instead. If the type annotations are in comments, is the |
|
If everything you import from typing is hidden inside a MYPY = False
if MYPY:
from typing import Any, .....This hides the import at runtime but MYPY itself processes the block as if |
|
I'm fine with the typeshed stub. I don't want annotations in my own sourcetree that incur any runtime cost. |
Authorized by atomicwrites author here: untitaker/python-atomicwrites#23 (comment)
Authorized by atomicwrites author here: untitaker/python-atomicwrites#23 (comment)
So, I've been playing with the MyPy static type checker lately, and I decided spend some downtime adding type annotations to Python projects I've worked on.
I had to change the logic a bit in order to make the code type check successfully, mainly by more eagerly converting a "unicode or bytes" path to unambiguously unicode when the object is constructed.
I also made it so that
tox -e py-typecheckwill run the type checker.