-
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: Finish type hints and mark package as typed #3272
Conversation
…Already removed for layer and hconcat in a previous PR
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 |
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. |
Thanks! Shall we make |
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! 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.
Fully agree, thanks for catching that. |
Nice! One more question, what is the difference between |
I see already: DataType = Union[dict, pd.DataFrame, SupportsGeoInterface, DataFrameLike]
ChartDataType = Union[DataType, core.Data, str, core.Generator, UndefinedType]
|
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.
Congrats @binste! A lot of hard work! Thanks. Let's start testing this within real use cases in the coming period.
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 :) |
This will close #2951 🥳