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

add typehint Any to Undefined object #2670

Closed
wants to merge 1 commit into from

Conversation

brahn
Copy link
Contributor

@brahn brahn commented Aug 17, 2022

This PR adds the typehint Any to the undefined object, suppressing the many type errors described in microsoft/pylance-release#3210

Candidly this is not a great solution and hardly sufficient to address #2493 (I think a full set of type hints like #2614 would be preferable) but it at least this removes the distraction.

Very open to alternatives. Thanks for your consideration!

cc: @thewchan

@arogozhnikov
Copy link

arogozhnikov commented Sep 19, 2022

Can someone please have a look at this?

Right now checking code with pylance (vs code default) or pyright (same thing really) produces an error for every argument in every call in altair.
As you can imagine, that's a lot of noise

I'm not exactly sure if this (auto-assumption that argument has a type of its default value) is the PEP-compatible behavior, but this simple trick proposed by @brahn probably can solve the issue.

@mattijn
Copy link
Contributor

mattijn commented Sep 20, 2022

Thanks for the ping @arogozhnikov! Do you know anyone who can perform a review regarding the dos and do nots for types to push this forward?
In linguistics terms, I disagree that Any equals Undefined, but maybe this is different in Type-land?

@brahn
Copy link
Contributor Author

brahn commented Sep 20, 2022

In linguistics terms, I disagree that Any equals Undefined, but maybe this is different in Type-land?

I agree this reads a bit odd a bit odd at first! To provide a bit of background:

  • Any is a python type-hint. From the Python docs:

    Special type indicating an unconstrained type. Every type is compatible with Any. Any is compatible with every type.

  • The Undefined object, defined by Altair, is an instance of the UndefinedType class (also defined by Altair, just a few lines prior to the proposed change).

To explain why this change helps, it's useful to look at an example like the following https://github.com/altair-viz/altair/blob/fc7f5510d189303aa58e751cb5876fb4929f36c8/altair/vegalite/v5/schema/mixins.py#L11-L15

The arguments to this function all have the Undefined object as their default value. Unfortunately, the python type-checker wrongly concludes from this default value that all arguments should be instances of the same class as Undefined, i.e. it expects instances of the UndefinedType class -- which is not what we really want.

By adding the type hint that Undefined should be considered an Any (for typing purposes), we correct the type-checker's logic and explain that it should not object to other values passed to these arguments.

@arogozhnikov
Copy link

arogozhnikov commented Sep 20, 2022

@brahn made a nice summary. My two cents:

From user perspective, I don't see a case when this can turn into a problem.
It does not provide any meaningful checks for arguments, but at least fixes wrong type-checker assumptions about type.

Looking forward, at some point I expect altair to implement type-checking for at least some most common arguments. At that point you'll have three options:

  • put :Any = undefined for every argument of every function
  • or implement the change in this PR
  • for every type checker find a module-level configuration that switches off default-defines-type behavior. There are may type checkers just in case and I'm not sure this is possible at all

I'm quite certain that second option is preferred.
Starting from there, you can annotating argument with correct types, selectively, step-by-step.

@mattijn
Copy link
Contributor

mattijn commented Sep 20, 2022

As usual, things are not that easy. The essence of this issue has been, albeit not directly named, discussed in https://twitter.com/jakevdp/status/1557778437493755904 & https://twitter.com/jakevdp/status/1518659999844630528.
Based on this one could state that UndefinedLike = Any as an alias which would make most of the people a bit happy.

@brahn
Copy link
Contributor Author

brahn commented Sep 20, 2022

@mattijn am I following correctly that you'd like to see something like this? Happy to update the PR with this change

https://github.com/altair-viz/altair/blob/c4248a953c8518efb3cf5bef8d9755bd0fc7391e/altair/utils/schemapi.py#L141-L148

@brahn
Copy link
Contributor Author

brahn commented Sep 22, 2022

closing in favor of #2681

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.

None yet

3 participants