Add typehints to fsspec.mapping and fsspec.dircache
#1396
Conversation
fsspec/dircache.py
Outdated
| type: str | ||
|
|
||
|
|
||
| class DirCache(MutableMapping[str, List[DirEntry]]): |
There was a problem hiding this comment.
This class doesn't ever specify the key and value types, so theoretically I could make this a generic type. The docstrings imply this is the right type though?
| def __contains__(self, item: object) -> bool: | ||
| try: | ||
| self[item] | ||
| self[item] # type: ignore[index] |
There was a problem hiding this comment.
This is a little unfortunate -- the MutableMappign typehints say that __contrains__ should take all objects, not just the key type, hence the type ignore here.
There was a problem hiding this comment.
I hate this kind of inconsistency :|
There was a problem hiding this comment.
Yeah, I don't love it either.. it's not great.
There was a problem hiding this comment.
You can around this by adding an assert isinstance here.
|
|
||
| def getitems( | ||
| self, | ||
| keys: Iterable[str], |
There was a problem hiding this comment.
I could keep this as a list if we want to add that restriction.
| """Remove all keys below root - empties out mapping""" | ||
| try: | ||
| self.fs.rm(self.root, True) | ||
| self.fs.mkdir(self.root) | ||
| except: # noqa: E722 | ||
| pass | ||
|
|
||
| def getitems(self, keys, on_error="raise"): | ||
| @overload |
There was a problem hiding this comment.
This is a bit of ugliness to make the output type of getitems dynamic based on on_error
There was a problem hiding this comment.
Can we not just use str and ignore the overload? I don't think this kind of construct is worth it.
fsspec/dircache.py
Outdated
| type: str | ||
|
|
||
|
|
||
| class DirCache(MutableMapping[str, List[DirEntry]]): |
| def __contains__(self, item: object) -> bool: | ||
| try: | ||
| self[item] | ||
| self[item] # type: ignore[index] |
There was a problem hiding this comment.
I hate this kind of inconsistency :|
| """Remove all keys below root - empties out mapping""" | ||
| try: | ||
| self.fs.rm(self.root, True) | ||
| self.fs.mkdir(self.root) | ||
| except: # noqa: E722 | ||
| pass | ||
|
|
||
| def getitems(self, keys, on_error="raise"): | ||
| @overload |
There was a problem hiding this comment.
Can we not just use str and ignore the overload? I don't think this kind of construct is worth it.
Continuation of #1284, but add type hints to
fsspec.mappingandfsspec.dircache.