Skip to content

Conversation

@Stannislav
Copy link
Contributor

@Stannislav Stannislav commented Jul 31, 2021

This fixes a wrong type in the PyYAML stubs.

The last parameter in yaml.add_constructur(tag, constructor, Loader) is the loader class and not its instance. This is consistent with both the PyYAML docs and the way it's implemented in the PyYAML package.

In fact, since the only requirement on the Loader parameter is to have the Loader.add_constructor class method, which is defined in the BaseConstructor base class, we should use the corresponding base class type Type[BaseConstructor]

Simlar types can be added to the similar functions in the same block:

  • def add_implicit_resolver(...)
  • def add_path_resolver(...)
  • def add_constructor(...)
  • def add_multi_constructor(...)
  • def add_representer(...)
  • def add_multi_representer(...)

@JelleZijlstra
Copy link
Member

Thank you! Feel free to change the other instances of this issue too.

In the implementation of `yaml.add_constructor` the only requirement
on the `Loader` parameter is that it defines the
`Loader.add_constructor` class method. This method is defined in
the `BaseConstructor` base class. So we should use this base class
as type parameter type.
@Stannislav Stannislav changed the title Change Loader to Type[Loader] in yaml.add_constructor Change Loader to Type[BaseConstructor] in yaml.add_constructor Jul 31, 2021
Stanislav Schmidt added 4 commits July 31, 2021 17:03
Added / modified types for the following functions:
- `add_implicit_resolver(...)`
- `add_path_resolver(...)`
- `add_constructor(...)`
- `add_multi_constructor(...)`
- `add_representer(...)`
- `add_multi_representer(...)`

For the last two only some formatting was changed.
@Stannislav
Copy link
Contributor Author

Hi @JelleZijlstra, thanks for having a look. I just added annotations to the other functions mentioned in the description.

It would be good if someone more experience than me in both typing and the yaml-library could glance over it, just to see if all makes sense.

Also, I'm not 100% sure if writing something like Type[Any] in add_path_resolver makes sense.

Stanislav Schmidt added 2 commits August 1, 2021 09:38
We need to use TypeVar in order to match the types in the
second parameter (construtor) and third parameter (Loader). Setting
`constructor: Callable[[BaseConstructor, None], Any]` and
`Loader: OptionalType[BaseConstructor]` doesn't work for when
`Loader` is a subclass `C` of `BaseConstructor`. This is because
`Callable[[C, None], Any]` cannot be substituted for
`Callable[[BaseConstructur], Any]`.
@Akuli
Copy link
Collaborator

Akuli commented Aug 8, 2021

We changed all stubs to use the new foo | None syntax instead of Optional[foo] in #5872, and that conflicts with this PR. To fix it, pull from our master and then resolve the conflicts:

$ git pull https://github.com/python/typeshed

Please also use the newer foo | None syntax in this PR.

Also, I'm not 100% sure if writing something like Type[Any] in add_path_resolver makes sense.

It makes sense. PyYAML checks whether it is str or list or some other types.

@Stannislav
Copy link
Contributor Author

We changed all stubs to use the new foo | None syntax instead of Optional[foo] in #5872, and that conflicts with this PR. To fix it, pull from our master and then resolve the conflicts:

$ git pull https://github.com/python/typeshed

Please also use the newer foo | None syntax in this PR.

Also, I'm not 100% sure if writing something like Type[Any] in add_path_resolver makes sense.

It makes sense. PyYAML checks whether it is str or list or some other types.

Hi @Akuli, thanks for reviewing. Merged master in bc77aed and also addressed your comment in the thread above.

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Akuli Akuli merged commit 64903b8 into python:master Aug 8, 2021
@Stannislav Stannislav deleted the fix-yaml-add-constructor branch August 8, 2021 18:52
Stannislav pushed a commit to scanapi/scanapi that referenced this pull request Aug 22, 2021
Version 5.4.6 contains a fix for wrong type annotations. The fix
and additional type annotations were added in this PR:
python/typeshed#5828
Stannislav pushed a commit to scanapi/scanapi that referenced this pull request Aug 22, 2021
Version 5.4.6 contains a fix for wrong type annotations. The fix
and additional type annotations were added in this PR:
python/typeshed#5828
Stannislav pushed a commit to scanapi/scanapi that referenced this pull request Aug 23, 2021
Version 5.4.6 contains a fix for wrong type annotations. The fix
and additional type annotations were added in this PR:
python/typeshed#5828
Stannislav added a commit to scanapi/scanapi that referenced this pull request Aug 23, 2021
Version 5.4.6 contains a fix for wrong type annotations. The fix
and additional type annotations were added in this PR:
python/typeshed#5828
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants