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: Finish type hints and mark package as typed #3272

Merged
merged 8 commits into from
Nov 23, 2023

Conversation

binste
Copy link
Contributor

@binste binste commented Nov 22, 2023

This will close #2951 🥳

  • Build wheel file and test it on a larger Altair code base

@binste binste marked this pull request as ready for review November 22, 2023 18:32
@mattijn
Copy link
Contributor

mattijn commented Nov 22, 2023

Thanks @binste! Can you briefly explain how these new public types can be used from outside Altair? Would it be possible to import certain types from Altair and do some checks without defining a Chart object?

@binste
Copy link
Contributor Author

binste commented Nov 23, 2023

Sure! For example, if you have a function which creates a chart object based on some inputs, it would be great to make sure to test that the passed in data is something that Altair can deal with:

import pandas as pd
import altair as alt


def make_line_chart(data: alt.ChartDataType, x: str, y: str) -> alt.Chart:
    # Imagine that this function creates a more complicated chart spec
    return alt.Chart(data).mark_line().encode(x=x, y=y)


make_line_chart(data=pd.DataFrame(), x="a", y="b")  # This passes
make_line_chart(
    data=2, x="a", y="b"
)  # error: Argument "data" to "make_line_chart" has incompatible type "int"; expected "dict[Any, Any] | DataFrame | _SupportsGeoInterface | _DataFrameLike | Data | str | Generator | UndefinedType"  [arg-type]

For most users, these types/type aliases are probably irrelevant but if you build an application or framework which wants to tightly integrate with Altair, I think they can come in handy.

@mattijn
Copy link
Contributor

mattijn commented Nov 23, 2023

Thanks! Shall we make _SupportsGeoInterface and _DataFrameLike public too? Seems useful.

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

Thanks! Shall we make _SupportsGeoInterface and _DataFrameLike public too? Seems useful.

I agree. I think _DataFrameLike should be public since it's the return type of chart.transformed_data(). We expose _SupportsGeoInterface as part of the Union defining the public DataType type, so I think it should be public as well.

pyproject.toml Show resolved Hide resolved
@binste
Copy link
Contributor Author

binste commented Nov 23, 2023

Fully agree, thanks for catching that.

@mattijn
Copy link
Contributor

mattijn commented Nov 23, 2023

Nice! One more question, what is the difference between ChartDataType and DataType?

@mattijn
Copy link
Contributor

mattijn commented Nov 23, 2023

I see already:

DataType = Union[dict, pd.DataFrame, SupportsGeoInterface, DataFrameLike]

ChartDataType = Union[DataType, core.Data, str, core.Generator, UndefinedType]

DataType is a subset of ChartDataType

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.

Congrats @binste! A lot of hard work! Thanks. Let's start testing this within real use cases in the coming period.

@binste
Copy link
Contributor Author

binste commented Nov 23, 2023

Thanks! Yeah it was a longer journey but also very educational for me and I hope it's of use. I already tested it on a medium-sized web application which uses many Altair charts and it allowed me to discover some edge cases in the application code which could have led to bugs :)

@binste binste merged commit dd5b61a into vega:main Nov 23, 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.

Type hints: Summary issue
3 participants