Type annotations for base spec#2041
Conversation
| maxdepth: int | None, | ||
| topdown: bool, | ||
| on_error: Literal["omit", "raise"] | Callable[[OSError], Any], | ||
| detail: Literal[True], |
There was a problem hiding this comment.
does it makes sense to make detail kwarg explicit (with default false) for all the functions that takes it?
| ) | ||
| else: | ||
| ac = kwargs.pop("autocommit", not self._intrans) | ||
| ac: bool = kwargs.pop("autocommit", not self._intrans) |
There was a problem hiding this comment.
I would like to make these kwargs explicit with default None. What do you think?
There was a problem hiding this comment.
The point was, not all subclasses support autocommit - so they would have to have their methods changed to also have the explicit kwarg and then ignore it.
martindurant
left a comment
martindurant
left a comment
There was a problem hiding this comment.
Some thoughts.
My biggest gripe is the extended overrides, mostly because of detail=True|False.
| async_impl: bool = False | ||
| mirror_sync_methods: bool = False |
There was a problem hiding this comment.
These are class attributes
| self._invalidated_caches_in_transaction.append(path) | ||
|
|
||
| def mkdir(self, path, create_parents=True, **kwargs): | ||
| def mkdir(self, path: TPath, create_parents: bool = True, **kwargs: Any): |
There was a problem hiding this comment.
Annotating Any doesn't really help...
There was a problem hiding this comment.
Yeah but without it the type is Unknown and mypy raises warning for that
| @overload | ||
| def ls( | ||
| self, path: TPath, detail: Literal[True], **kwargs: Any | ||
| ) -> list[FileInfo]: ... | ||
|
|
||
| @overload | ||
| def ls(self, path: TPath, detail: Literal[False], **kwargs: Any) -> list[str]: ... | ||
|
|
||
| def ls( | ||
| self, path: TPath, detail: bool = True, **kwargs: Any | ||
| ) -> list[str] | list[FileInfo]: |
There was a problem hiding this comment.
I realise that you are trying to specify: if detail is True, returns list[dict]; if False returns list[str]. Does it really take all this code to do so?
There was a problem hiding this comment.
sadly yes. python doesn't have proper type manipulation support like typescript yet.
| fs: AbstractFileSystem, | ||
| path: str, | ||
| mode: str = "rb", | ||
| block_size: int | Literal["default"] | None = "default", | ||
| autocommit: bool = True, | ||
| cache_type: Literal["readahead", "none", "mmap", "bytes"] = "readahead", | ||
| cache_options: dict[str, Any] | None = None, | ||
| size: int | None =None, |
There was a problem hiding this comment.
General question: why is it useful to list the attributes above and also type them here, conveying the information twice?
There was a problem hiding this comment.
these are two separate typehints one for class members and one for constructor function. python type system can't infer it from constructor function types
| mode: str = "rb", | ||
| block_size: int | Literal["default"] | None = "default", | ||
| autocommit: bool = True, | ||
| cache_type: Literal["readahead", "none", "mmap", "bytes"] = "readahead", |
There was a problem hiding this comment.
This list is not exhaustive, more cache types may be registered at runtime (see the "prefetcher" cache in gcsfs and "known_parts" cache in fsspec.parquet; also "all" is missing).
| self.DEFAULT_BLOCK_SIZE if block_size in ["default", None] else block_size | ||
| self.DEFAULT_BLOCK_SIZE if block_size is None or block_size == "default" else block_size |
There was a problem hiding this comment.
because type checker can't narrow types with in operator.
| @property | ||
| def name(self): | ||
| return _unstrip_protocol(self.path, self.fs) |
There was a problem hiding this comment.
Where does this new property come from?
| else: | ||
| raise ValueError("Info not available while writing") | ||
|
|
||
| @override |
|
@martindurant Thanks for review! I think I should've started with type stubs then later maybe add inline types. type stubs will be cleaner to add than inline types. I will push type stubs this weekend. |
|
@martindurant I have added completely separate type stub file. I am planning to add more for known implementations once you review this |
|
Much happier with the annotations separate :). Now the main code is not affected, nor even import times. Do you have any thoughts on how best to keep the code and types in sync in the future? |
|
mypy has https://mypy.readthedocs.io/en/stable/stubtest.html#automatic-stub-testing-stubtest |
|
Can you please draft a non-required CI run with that, and text instructions for how to go about updating types in the future as the API changes? |
2 participants
Fixes #625