Type annotate storage #362
Conversation
|
Mentioning @joshuagl in case he can remember something that I have missed in the context manager implementation. |
securesystemslib/storage.py
Outdated
| @@ -200,7 +200,7 @@ def get(self, filepath:str) -> Generator[BinaryIO, None, None]: | |||
| try: | |||
| file_object = open(filepath, 'rb') | |||
| yield file_object | |||
| except (FileNotFoundError, IOError): | |||
| except FileNotFoundError: | |||
There was a problem hiding this comment.
I think this went the wrong way and should be OSError now -- to cover things like IsADirectoryError
There was a problem hiding this comment.
You have a point, updated in 32ab712
sechkova
force-pushed
the
type-annotate-storage
branch
2 times, most recently
from
8fac1f3 to
32ab712
Compare
There was a problem hiding this comment.
Mentioning @joshuagl in case he can remember something that I have missed in the context manager implementation.
This looks great. It may have been a Python 2.7 limitation or just my not finding the right part of the docs when I implemented it without the decorator.
securesystemslib/storage.py
Outdated
| @@ -37,7 +38,8 @@ class StorageBackendInterface(): | |||
|
|
|||
|
|
|||
| @abc.abstractmethod | |||
| def get(self, filepath): | |||
| @contextmanager | |||
| def get(self, filepath: str) -> Generator[BinaryIO, None, None]: | |||
There was a problem hiding this comment.
I think we can just use Iterator[None] for the return type here?
See https://stackoverflow.com/questions/49733699/python-type-hints-and-context-managers and https://github.com/python/typeshed/blob/master/stdlib/contextlib.pyi#L34
There was a problem hiding this comment.
if that works (and that would make sense) then I suppose it's Iterator[BinaryIO]?
There was a problem hiding this comment.
That would make more sense, indeed.
securesystemslib/storage.py
Outdated
| @@ -193,7 +195,7 @@ def __new__(cls, *args, **kwargs): | |||
|
|
|||
|
|
|||
| @contextmanager | |||
| def get(self, filepath): | |||
| def get(self, filepath:str) -> Generator[BinaryIO, None, None]: | |||
There was a problem hiding this comment.
Same comment on return type here.
|
Added one commit with the proposal. Checked and |
|
Just realised that you'll need to add storage.py to the list of files in mypy.ini |
In order to enable static type checking the types of the abstract method 'get' and its concrete implementation have to match. Assining GetFile class to a callable leads to type errors. Signed-off-by: Teodora Sechkova <[email protected]>
Gradually enable static type checking. Signed-off-by: Teodora Sechkova <[email protected]>
Since Python 3.3 IOError is an alias of OSError and is kept for compatibility. No longer needed since the project supports only bigger Python versions. Signed-off-by: Teodora Sechkova <[email protected]>
The @contextmanager decorator returns an Iterator[BinaryIO] which a generalisation of Generator[BinaryIO, None, None]. Signed-off-by: Teodora Sechkova <[email protected]>
Add storage.py to the list of type annotated files checked by mypy. Signed-off-by: Teodora Sechkova <[email protected]>
sechkova
force-pushed
the
type-annotate-storage
branch
from
e04ddaa to
35c5413
Compare
Rebased on top of the main branch and added. |
Addresses #358
Description of the changes being introduced by the pull request:
Adds type annotations to
storage.py.In addition changes
FilesystemBackend.getto avoidmypycomplaining (correctly) about the following assignment:Please verify and check that the pull request fulfils the following requirements: