🐛 FIX: aiida/repository typing #4920
Conversation
Fixes 54 errors and one identified bug (setting directory)
aiida/repository/repository.py
Outdated
| @@ -32,7 +32,7 @@ class keeps a reference of the virtual file hierarchy. This means that through t | |||
|
|
|||
| # pylint: disable=too-many-public-methods | |||
|
|
|||
| _backend = None | |||
| _backend: Optional[AbstractRepositoryBackend] = None | |||
There was a problem hiding this comment.
why does this need to be a class attribute and not a regular one?
There was a problem hiding this comment.
In principle it doesn't, we initialize it in the constructor anyway, so I think we can get rid of it.
aiida/repository/repository.py
Outdated
| @@ -161,7 +162,7 @@ def _insert_file(self, path: pathlib.Path, key: str): | |||
| if path.parent: | |||
| directory = self.create_directory(path.parent) | |||
| else: | |||
| directory = self.get_directory | |||
| directory = self.get_directory() | |||
There was a problem hiding this comment.
this was a definite bug
There was a problem hiding this comment.
should we add the missing test perhaps then?
There was a problem hiding this comment.
ok I'll have a look 👍
There was a problem hiding this comment.
How was this intented to work? I think its actually impossible to reach that code, given that Path().parent == Path(). Is that perhaps what the test should be?
There was a problem hiding this comment.
or shall I just remove the if/else?
There was a problem hiding this comment.
I think the conditional makes sense it is just incorrect. What I think I intended here is to check whether the parent directory of the path exists. If not, then we create it first. Otherwise, we just get the directory.
That being said, "creating" directories here is all just virtual, and there aren't actual directories being explicitly created in the backend repository, just the virtual hierarchy. Since create_directory should only create the directory if it doesn't already exist, we could always just call it and don't need to check. So I guess we can indeed just get rid of the entire conditional and just call self.create_directory(path.parent).
There was a problem hiding this comment.
yeh thats what I thought, removed
It is also kind of pointless to be complaining about PRs that you approved yourself |
😆 I didn't notice that it was not added to the list (you were half way there), but yeh I do want to push that people do this, because there is a definite benefit |
| @@ -81,7 +89,7 @@ def has_object(self, key: str) -> bool: | |||
| """ | |||
|
|
|||
| @contextlib.contextmanager | |||
| def open(self, key: str) -> io.BufferedIOBase: | |||
| def open(self, key: str) -> typing.Iterator[typing.BinaryIO]: # type: ignore[return] | |||
There was a problem hiding this comment.
hmmm, these abstract methods, that you also need to call super() on, mypy is not a fan 😬
| """Store the byte contents of a file in the repository. | ||
|
|
||
| :param handle: filelike object with the byte content to be stored. | ||
| :return: the generated fully qualified identifier for the object within the repository. | ||
| """ | ||
| if not isinstance(handle, io.BytesIO) and not self.is_readable_byte_stream(handle): |
There was a problem hiding this comment.
mypy didn't like this, and it feels like it goes a bit against the concept of an abstract method, so I moved it to a separate method. But its not a deal breaker
There was a problem hiding this comment.
I guess the best way to do this technically is something like:
def put_object_from_filelike(self, handle: typing.BinaryIO) -> str:
"""Store the byte contents of a file in the repository.
:param handle: filelike object with the byte content to be stored.
:return: the generated fully qualified identifier for the object within the repository.
"""
check_byte_stream(handle)
return _put_object_from_filelike(handle)
@abc.abstractmethod
def _put_object_from_filelike(self, handle: typing.BinaryIO) -> str:then the concrete methods do not have to "remember" to call super() or any other initial code
Codecov Report
@@ Coverage Diff @@
## develop #4920 +/- ##
===========================================
+ Coverage 80.05% 80.06% +0.01%
===========================================
Files 515 515
Lines 36612 36622 +10
===========================================
+ Hits 29306 29316 +10
Misses 7306 7306
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Cheers @chrisjsewell , all good, except I would suggest to add a test for the __str__ method of initialized and uninitialized repos because that was clearly a bug that went uncaught. And I have a question about the conditionals checking File.key is None.
aiida/repository/repository.py
Outdated
| key = self.get_file(path).key | ||
| if key is None: | ||
| raise TypeError('File key not set') |
There was a problem hiding this comment.
So I see why mypy thinks it needs this, because technically File.key can indeed return str or None. However, if the File has file_type==FileType.File the key cannot be None because this is checked in the constructor. So we should never get in this TypeErrorr since if key is None than get_file will already have raised IsADirectoryError because it is not a file. So I am wondering if this should not simply be an AssertionError if we really need to have this check here, because it would be an internal coding inconsistency.
There was a problem hiding this comment.
done 👍
yeh this is why I personally probably would have erred towards having File and Directory subclasses, but appreciate it is somewhat a matter of personal preference
There was a problem hiding this comment.
There might be a problem on my side, but I still see the TypeError. Did you change this to
assert self.get_file(path).key is not None
?
There was a problem hiding this comment.
Oh yeh, I changed it in delete_object, but not here
aiida/repository/backend/sandbox.py
Outdated
|
|
||
| def __str__(self) -> str: | ||
| """Return the string representation of this repository.""" | ||
| return f'SandboxRepository: {self._sandbox.abspath}' | ||
| return f'SandboxRepository: {self._sandbox.abspath if self._sandbox else "null"}' |
There was a problem hiding this comment.
This was clearly a bug and not caught by the tests. I think it would be good to add a quick test, both for the sandbox and diskobject backends.
There was a problem hiding this comment.
jeez, and yet you can't be bothered to add type checking to migrations 😛
There was a problem hiding this comment.
done; I adapted them slightly, to return SandboxRepository: <uninitialised>/DiskObjectStoreRepository: <uninitialised> before initialisation
There was a problem hiding this comment.
Thanks @chrisjsewell almost there, just don't see the change in the TypeError raising, but your comment suggests you did change it.
aiida/repository/repository.py
Outdated
| key = self.get_file(path).key | ||
| if key is None: | ||
| raise TypeError('File key not set') |
There was a problem hiding this comment.
There might be a problem on my side, but I still see the TypeError. Did you change this to
assert self.get_file(path).key is not None
?
Well yeh, the other way I would word it is that Personally I would also consider changing the |
Fixes 54 errors and one identified bug (setting directory)
@sphuber its kind of pointless adding types, if you don't also type check them 😬