-
Notifications
You must be signed in to change notification settings - Fork 795
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
Type hints: Infer types from vegalite schema for autogenerated code #3208
Conversation
…e has multiple arguments with Undefined default value
…o not allow for combinations of different types in lists. This deals with the invariance of lists
…ect that the overload statements are correct
This reverts commit 54439c8.
…move type hints from signatures in docs as they are already shown in docstrings
We'll need to first implement the conversion of anything that looks like a Sequence (or just any iterable?) to a list as in the Vega-Lite specification we need them to be lists (i.e. an |
Side note, please ignore if not related, is this related to this issue: #2877? Would it be possible to define a type for SchemaValidationError: 'range(-15, 46, 60)' is an invalid value for `domain`. Valid values are of type 'array', 'string', or 'object'.
'unaggregated' was expected Or is this merely a matter of converting an |
It means that wherever
and use this as a type hint: class A:
def get_number(self) -> int:
return 2
def other_method(self) -> str:
return "something"
def extract_number(x: Proto) -> int:
return x.get_number()
a = A()
extract_number(a) # Mypy will accept this As you mentioned, here I had to use it to prevent circular imports as ideally, we could use
We could use |
Thanks for the clear explanation @binste. Much appreciated! I feel there is a balance between how we suggest/hint how users should use the library and what is function-wise the correct type hint. It is great that we can make our type checker happy, but I'm hoping we also can use these type hints to be useful in communicating how users can use the altair API. I feel the battle that you've had with mypy (much appreciated!), but I'm trying to review as being an average user.. For my understanding, would it be possible to make a small example how this currently leads to circular imports? I'm somehow hoping we can circumvent this definition of |
That's much appreciated and exactly what this PR needs! It's easy to get too focused on just adding type hints everywhere and making mypy happy so I think the feedback from Joel and you have already improved this a lot :)
Sure! I hope as well that there is a feasible solution which I'm missing. To keep it maintainable, I think code should be imported from |
Thanks again for the extra explanations. I start to understand the issue. Would it be possible to define an abstract base class of the
|
Thanks for the example and playing around with it! I can replicate the mypy errors when implementing it in this PR and I can't figure out what the issue is with this approach... It's not due to the naming of the class, I can rename the protocol to I assume the improvement would be that How about renaming it to |
Thanks @binste! Can you do the same for the I was playing around and checking the type hints here and there (it's a lot, but informative!), but when I check the type hints of |
The Regarding But good that you bring it up, I realised that if I change the import statement to I only used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the hard work here @binste! I approve:).
Since this involves the Altair API's user-facing side, having an additional review would be much appreciated. No rush, but would you be willing to have a look to his @jonmmease?
Thanks @mattijn for the very helpful review! :) Sounds good, I'll see if someone else finds a moment to have a look at the changes and in the meantime, I'll already try to prepare the final changes in a separate PR so that we can mark Altair as typed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks for the epic work on this @binste! This is a great step forward for Altair as a foundational library in the PyData ecosystem.
After playing with the branch for a bit, I only came up with one non-blocking suggestion:
Would it be possible to replace BinParams
, ImputeParams
, TitleParams
with the shorter Bin
, Impute
, and Title
aliases?
Thanks @jonmmease for the kind words and the review! :) I agree that this would be nicer. Unfortunately, these aliases are defined in |
This will close #2854. See #2951 for an overview of the type hint effort