Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update pyvmomi to address #8082 and use Incomplete type instead of Any #8469

Merged
merged 7 commits into from
Aug 3, 2022
Merged

Update pyvmomi to address #8082 and use Incomplete type instead of Any #8469

merged 7 commits into from
Aug 3, 2022

Conversation

kkirsche
Copy link
Contributor

@kkirsche kkirsche commented Aug 2, 2022

fix: #8082 RE: inherit from Exception

Use Incomplete instead of Any where # incomplete comments were in place.

@srittau
Copy link
Collaborator

srittau commented Aug 2, 2022

I haven't lookup at the pytype error, but LGTM apart from that.

@github-actions

This comment has been minimized.

@kkirsche
Copy link
Contributor Author

kkirsche commented Aug 2, 2022

I'm not honestly sure what's happening in pytype to cause this error:

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/site-packages/pytype/load_pytd.py", line 327, in resolve_external_types
    mod_ast = mod_ast.Visit(visitors.LookupExternalTypes(
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/site-packages/pytype/pytd/parse/node.py", line [99](https://github.com/python/typeshed/runs/7635144002?check_suite_focus=true#step:5:100), in Visit
    return _Visit(self, visitor, *args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/site-packages/pytype/pytd/parse/node.py", line 117, in _Visit
    return _VisitNode(node, visitor, *args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/site-packages/pytype/pytd/parse/node.py", line 222, in _VisitNode
    new_node = visitor.Visit(new_node, *args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/site-packages/pytype/pytd/base_visitor.py", line 203, in Visit
    return self.visit_functions[node.__class__.__name__](
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/site-packages/pytype/pytd/visitors.py", line 631, in VisitTypeDeclUnit
    new_aliases = self._HandleDuplicates(new_aliases)
  File "/opt/hostedtoolcache/Python/3.10.5/x64/lib/python3.10/site-packages/pytype/pytd/visitors.py", line 592, in _HandleDuplicates
    raise KeyError("Duplicate top level items: %r, %r" % (
KeyError: "Duplicate top level items: None, '_typeshed.Incomplete'"

I don't see a duplicate in this, but I'm not clear on the purpose of this test to really investigate it

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nit: for unknown argument annotations , we generally prefer just to leave them unannotated rather than annotating them with Incomplete or Any. It has slightly different semantics for some type checkers, such as pyright.

stubs/pyvmomi/pyVmomi/vmodl/query.pyi Outdated Show resolved Hide resolved
stubs/pyvmomi/pyVmomi/vmodl/query.pyi Outdated Show resolved Hide resolved
stubs/pyvmomi/pyVmomi/vmodl/query.pyi Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

I'm also not quite sure what the pytype error is trying to tell us. @rchen152, could you possibly take a look? :)

Kevin Kirsche and others added 4 commits August 2, 2022 15:05
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@kkirsche
Copy link
Contributor Author

kkirsche commented Aug 2, 2022

Nit: for unknown argument annotations , we generally prefer just to leave them unannotated rather than annotating them with Incomplete or Any. It has slightly different semantics for some type checkers, such as pyright.

Thanks for the clarification. As I understand it so far from other feedback, Any is meant to imply truly anything, but not to mean that it's unused (where Any would still be appropriate).

Where any object can be used due to not being used, object is instead used.

Incomplete seems to be the confusing bit at the moment as I would expect a resolved Unknown type in pyright to be equivalent to typeshed's Incomplete. Are there any docs about that nuance?

@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Aug 2, 2022

Your understanding is mostly correct.

There are two ways to look at this. From a type checker's point of view:

  • object means "could be anything, emit errors if the code assumes anything specific"
  • Any means "do not emit errors"
  • Incomplete means same as Any
  • A missing annotation is either same as Any, or if it's mypy and there's no annotations of parameters or return type, the function is considered to be untyped. As far as I understand it, pyright has Unknown for this but it behaves just like Any.

And from a typeshed maintainer's or contributor's point of view:

  • object means "could be anything, this function or method doesn't care"
  • Any means "can't be anything but it's impossible to express the situation properly with the features that type checkers have now"
  • Incomplete or a missing annotation means "can't be anything but it would be possible to do better than using Any". It is basically an implicit TODO comment.

I agree that this should be documented better, probably in CONTRIBUTING.md :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM

@srittau
Copy link
Collaborator

srittau commented Aug 2, 2022

Off topic, but a few more examples for object arguments:

  • An argument is just passed to str() or repr() or another function that accepts object.
  • Something like this:
def squeakable(it: object) -> bool:
    if not isinstance(o, Parrot):
        return False
    return it.squeak()

@rchen152
Copy link
Collaborator

rchen152 commented Aug 3, 2022

I have a fix for the pytype issue out for review. If all goes well, I should be able to release a new version of pytype with the fix on Wednesday or Thursday.

rchen152 added a commit to google/pytype that referenced this pull request Aug 3, 2022
…check.

This fixes a pytype test failure in
python/typeshed#8469.

PiperOrigin-RevId: 464935418
rchen152 added a commit that referenced this pull request Aug 3, 2022
Picks up a fix for the pytype test failure in
#8469.
@rchen152 rchen152 mentioned this pull request Aug 3, 2022
AlexWaygood pushed a commit that referenced this pull request Aug 3, 2022
Picks up a fix for the pytype test failure in
#8469.
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

kkirsche added a commit to kkirsche/ordered-set that referenced this pull request Aug 23, 2022
`Any` doesn't mean what it's being used here to indicate. Please see:
python/typeshed#8469 (comment)

For an explanation of what `Any` means to a type checker. This replaces the use of `Any` with `object`, which is the actual "any object" behavior that was intended.
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.

pyvmomi.vmodl: ManagedObjectNotFound should be derived from Exception
5 participants