Repository navigation
Add basic type annotations for YAML load functions - #1657
Conversation
| def load_all(stream, Loader=...): ... | ||
| def safe_load(stream): ... | ||
| def safe_load_all(stream): ... | ||
| def load(stream, Loader=...): ... # type: (Any, Loader) -> Any |
There was a problem hiding this comment.
It seems weird to use comment-based annotations here. Why not just
def load(stream: Any, Loader: Loader = ...) -> Any: ...
?
There was a problem hiding this comment.
Also, for the stream, could something like Union[str, IO[str]] work?
d6907d7 to
9a36414
Compare
These are probably the most common YAML functions, so this should be useful even without type declarations on the rest of the package.
9a36414 to
e784ef1
Compare
|
@matthiaskramm @JelleZijlstra do I need to make any further changes to get this merged? |
|
Looks good to me. Merging. |
| def load_all(stream, Loader=...): ... | ||
| def safe_load(stream): ... | ||
| def safe_load_all(stream): ... | ||
| def load(stream: Union[str, IO[str]], Loader: Loader=...) -> Any: ... |
There was a problem hiding this comment.
I discovered a problem with the annotation for Loader here. IIUC it's intended that this argument can be an instance of BaseLoader or SafeLoader as well. But given the inheritance structure in loader.pyi that doesn't work -- if I pass an instance of SafeLoader I get a type error. Maybe you can fix this by declaring this argument (here and below) as Union[BaseLoader, SafeLoader, Loader]? (Do double check that I got this right -- I'm not a user of yaml myself, but I found this caused a regression in an internal Dropbox codebase that uses it.)
There were two problems AFAICT: - There are multiple Loader classes without a common base class, so this should really be a union or a protocol. - The argument is supposed to be a class, not an instance. Rather than figuring out a fix (which would probably require some nasty refactoring) I'm just going back to the old situation where these arguments are implicitly annotated with `Any`. See also #1657 (review)
There were two problems AFAICT: - There are multiple Loader classes without a common base class, so this should really be a union or a protocol. - The argument is supposed to be a class, not an instance. Rather than figuring out a fix (which would probably require some nasty refactoring) I'm just going back to the old situation where these arguments are implicitly annotated with `Any`. See also #1657 (review)
4 participants
These are probably the most common YAML functions, so this should be
useful even without type declarations on the rest of the package.
This is my first contribution to typeshed, so please let me know if this patch makes sense. I tried to find a more specific type for the
streamargument (I know it supports both strings and file objects) but I couldn't find a definitive list of the types supported.