feat(sdk): collection files #46
Conversation
| def get_collection_file( | ||
| self, collection_id: str, file_key: str | ||
| ) -> Optional[CollectionFileReference]: | ||
| path = self._base_path / collection_id / f"{file_key}.json" |
There was a problem hiding this comment.
We need to ensure that files and documents do not overwrite eachother :)
| mock_response = MagicMock() | ||
| mock_response.status_code = 200 | ||
| mock_response.content = "" | ||
| mock_get.return_value = mock_response |
There was a problem hiding this comment.
Consider what a S3 download URL for a non-existing item would return (probably 404) 👍
|
|
||
|
|
|
|
A general comment: Rename |
| downloadURL=os.fspath(file.path), | ||
| uploadURL=os.fspath(file.path), |
There was a problem hiding this comment.
There is no upload/download in the file system client. This indicates that something about the implementation should fundamentally be changed.
There was a problem hiding this comment.
We should change the clients to not return graphql types, but our own types. That way we can do away with things like having a download URL related to a file on the file system.
There was a problem hiding this comment.
Basically, the clients should return NumerousFile and NumerousCollection objects directly. For files the handling of file content should be done by calls to the client internally.
For example there might just be similar methods in the client. Could be something like:
class NumerousFile:
...
def open(self) -> BufferedReader:
return self._client.open_file(self.id)Then FileSystemClient and GraphQLClient could implement Client.open_file in different ways.
| if self.file_id is not None: | ||
| tagged_file = self._client.add_collection_file_tag( | ||
| self.file_id, TagInput(key=key, value=value) | ||
| ) | ||
| else: | ||
| msg = "Cannot tag a non-existent file." | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Short circuit this as well, and also in tag_delete.
|
|
||
|
|
There was a problem hiding this comment.
Clean up extranous empty line
There was a problem hiding this comment.
Check all docstrings for this issue. It exists in multiple places.
No description provided.