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

Type hints: Improve for encoding channel attributes #2949

Merged

Conversation

binste
Copy link
Contributor

@binste binste commented Mar 6, 2023

This improves on the existing and very nice implementation done by @ChristopherDavisUCI.

  • Parse nested anyOf definitions which were so far represented as a single entry with non-specific **kwds.
  • I also added types for the items in lists, so instead of a generic list type hint we get more specific ones such as List[str]. Also works for altair classes.
  • Furthermore, it type hints enums as literals. This tells a type checker that only the specified values are accepted. Also helpful in VS Code:

image

All of the above changes together turn this:

   def sort(self, **kwds) -> 'Angle':

into this

    def sort(self, _: List[float], **kwds) -> 'Angle':
        ...

    @overload
    def sort(self, _: List[str], **kwds) -> 'Angle':
        ...

    @overload
    def sort(self, _: List[bool], **kwds) -> 'Angle':
        ...

    @overload
    def sort(self, _: List[core.DateTime], **kwds) -> 'Angle':
        ...

    @overload
    def sort(self, _: Literal["ascending", "descending"], **kwds) -> 'Angle':

In many cases, this is just an improvement over the existing type hints. However, for some such as X.title this fixes wrong type hints which will lead to errors when checking code with mypy. The old implementation does not correctly recognize that title can be of type str or List[str:

    @overload
    def title(self, **kwds) -> 'X':
        ...

    @overload
    def title(self, _: None, **kwds) -> 'X':
        ...

New:

    def title(self, _: str, **kwds) -> 'X':
        ...

    @overload
    def title(self, _: List[str], **kwds) -> 'X':
        ...

    @overload
    def title(self, _: None, **kwds) -> 'X':

See #2951 for an overview of all efforts related to type hints in Altair.

@mattijn
Copy link
Contributor

mattijn commented Mar 11, 2023

Thanks @binste, took me a while to notice that there are top and down arrows in the type hint so you can iterate through them. Here I'm at 3/5:
image
Which gives me hints on the LocalMultiTimeUnit from within the VL-schema. This is good.

But if I make a mistake in this, eg yearmonthdatehour instead of yearmonthdatehours

SchemaValidationError: 'yearmonthdatehour' is an invalid value for `timeUnit`:

'yearmonthdatehour' is not one of ['year', 'quarter', 'month', 'week', 'day', 'dayofyear', 'date', 'hours', 'minutes', 'seconds', 'milliseconds']
'yearmonthdatehour' is not one of ['utcyear', 'utcquarter', 'utcmonth', 'utcweek', 'utcday', 'utcdayofyear', 'utcdate', 'utchours', 'utcminutes', 'utcseconds', 'utcmilliseconds']

It seems it only returns the SingleTimeUnit anyOf as suggestion (LocalSingleTimeUnit and UtcSingleTimeUnit). This observation is maybe more connected to #2873 and not necessarily to this PR.

@binste
Copy link
Contributor Author

binste commented Mar 12, 2023

Nice catch! I propose a fix in #2957. Over time with a better understanding of the VL json schema, these error messages can hopefully be further improved.

@mattijn mattijn merged commit 539383a into vega:master Mar 14, 2023
@mattijn
Copy link
Contributor

mattijn commented Mar 14, 2023

Thanks!

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.

2 participants