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 hint api.py #3143

Merged
merged 23 commits into from
Oct 28, 2023
Merged

Type hint api.py #3143

merged 23 commits into from
Oct 28, 2023

Conversation

binste
Copy link
Contributor

@binste binste commented Aug 9, 2023

See #2951 for an overview of the type hinting efforts.

To dos:

  • Make sure that UndefinedType always comes last in Unions.
  • Parameter class is missing default values for attributes such as empty. See Add missing arguments to Parameter and add type hints #3173
  • Add Altair core classes to type hints as well. E.g. if a string for the protection type is accepted then probably also ProtectionType is accepted? E.g. FieldName in transform_ methods? Can see which classes are accepted by looking at what the underlying classes accept, e.g. for transform_density, check what DensitiyTransform accepts
    • These places might also accept dictionaries, e.g. if they just pass it through to another Altair core classes.
  • Need to add type hints for expressions expr (type hint as expr.core.Expression? How does it relate to ExprRef?) support for parameters which accept it such as projection. This is also not properly handled in the docstrings. Can view the underlying VL classes to see what they accept
    • If a string is accepted: Union[str, core.Expr, expr.core.Expression, UndefinedType]
    • If ExprRef: Union[core.ExprRef, UndefinedType]
  • Where are Parameter accepted?
    • value parameters possibly everywhere where we also accept ExprRef as a to_dict call produces the same output. E.g. mark_circle(opacity=alt.Parameter(...)).
    • selection parameters in e.g. transform_filter

@binste binste mentioned this pull request Aug 9, 2023
14 tasks
@mattijn
Copy link
Contributor

mattijn commented Aug 26, 2023

One question @binste.

I notice this will introduce things like type: Union[str, UndefinedType] = Undefined, for .project() arguments.

But in an example as such:

import altair as alt
from vega_datasets import data

source = alt.topo_feature(data.world_110m.url, 'countries')

input_dropdown = alt.binding_select(options=[
    "albers",
    "albersUsa",
    "azimuthalEqualArea",
    "azimuthalEquidistant",
    "conicEqualArea",
    "conicEquidistant",
    "equalEarth",
    "equirectangular",
    "gnomonic",
    "mercator",
    "naturalEarth1",
    "orthographic",
    "stereographic",
    "transverseMercator"
], name='Projection ')
param_projection = alt.param(value="equalEarth", bind=input_dropdown)

alt.Chart(source, width=500, height=300).mark_geoshape(
    fill='lightgray',
    stroke='gray'
).project(
    type=alt.expr(param_projection.name)
).add_params(param_projection)

The type argument is alt.expr(param_projection.name), which is not of type str but of type ExprRef, will that be fine with the approach in this PR?

@binste
Copy link
Contributor Author

binste commented Aug 26, 2023

Thanks for having a look @mattijn! Indeed, that is an outstanding item that I still need to tackle. I also need to add the acceptable Altair classes. For example core.ProjectionType for the type argument.

To make it more transparent what I still need to do, I moved the notes I had into the PR description.

@mattijn
Copy link
Contributor

mattijn commented Aug 26, 2023

Thanks for adding the notes. I understand this is hard to do correctly. Thank you for the clarification!

@binste binste marked this pull request as ready for review September 29, 2023 15:40
@binste
Copy link
Contributor Author

binste commented Sep 29, 2023

This is now ready to be reviewed. See the PR description for some more info on rules I tried to follow, etc. In general, I focused on public methods and functions and skipped some of the private ones as they would have been too much work for now.

I think these type hints will be very useful to users as they give a much more complete picture of what can be passed to various functions and methods, I definitely learned a lot doing this. However, I for sure have missed some types which would be accepted or maybe some hints are wrong. Given that it's so much code to cover, this is unavoidable but I think that's ok as it won't break working code and we can improve based on the feedback of users. Of course, if anyone already has inputs now, feel free to leave comments!

@mattijn
Copy link
Contributor

mattijn commented Oct 20, 2023

Thanks for all this work @binste! I noticed some of the functions within api.py will not contain type hints after this PR, like:

  • _consolidate_data
  • _prepare_data
  • _repr_mimebundle_
  • serve
  • _combine_subchart_data
  • remove_data
  • _needs_name
  • _prepare_to_lift
  • _remove_duplicate_params
  • _combine_subchart_params
  • _get_repeat_strings
  • _extend_view_name
  • _repeat_names
  • _remove_layer_props
  • remove_prop

Is this intended? Can a library be a typed library if not all functions contain type hints?

@binste
Copy link
Contributor Author

binste commented Oct 20, 2023

Thank you @mattijn for having a look! That is indeed intended. My goal right now is to have as much as possible of the "public API" of Altair typed and then incrementally do the rest later on if we see value in it.

A library can be declared "typed" at any point in time, even if it would not have any type hints. It just signals to type checkers such as mypy that they can use any type hints that they might discover in the project. In general, type hinting in Python can be done gradually. The more hints you have, the more useful a type checker is, but it's perfectly fine to leave stuff out if it's too complicated/not worth the effort :)

altair/vegalite/v5/api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mattijn mattijn left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications @binste. I'm approving with one little suggested change to place UndefinedType in the end as is done elsewhere.

@binste
Copy link
Contributor Author

binste commented Oct 28, 2023

Thank you @mattijn for the review! Merging 🥳

@binste binste merged commit 19eac93 into vega:main Oct 28, 2023
10 checks passed
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