-
Notifications
You must be signed in to change notification settings - Fork 793
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
feat: Adds ChartType
type and type guard is_chart_type
. Change PurePath to Path type hints
#3420
Conversation
I'm working on a library that uses `altair`, and looking to make it an optional dependency. During this process, I thought `ChartType` and `is_chart_type` might be a good fit in `altair` itself. I don't believe any existing `altair` code would directly benefit from these additions - but you could likely reduce some `isinstance` checks with [type narrowing](https://typing.readthedocs.io/en/latest/spec/narrowing.html) functions like `is_chart_type`. For example `altair/vegalite/v5/api.py` has **63** occurrences of `isinstance`. --- per [CONTRIBUTING.md](https://github.com/vega/altair/blob/main/CONTRIBUTING.md) > In particular, we welcome companion efforts from other visualization libraries to render the Vega-Lite specifications output by Altair. We see this portion of the effort as much bigger than Altair itself Towards this aim, it could be beneficial to provide `typing` constructs/tools for downstream use. Regarding my specfic use case, the below [Protocol](https://typing.readthedocs.io/en/latest/spec/protocol.html#runtime-checkable-decorator-and-narrowing-types-by-isinstance) - I believe - describes the concept of a `_/Chart`, for the purposes of writing to file: ```py from typing import Any, Protocol, runtime_checkable @runtime_checkable class AltairChart(Protocol): data: Any def copy(self, *args: Any, **kwargs: Any) -> Any: ... def save(self, *args: Any, **kwargs: Any) -> None: ... def to_dict(self, *args: Any, **kwargs: Any) -> dict[Any, Any]: ... def to_html(self, *args: Any, **kwargs: Any) -> str: ... ``` Due to the number of symbols in `altair` and heavy use of mixins, coming to this conclusion can be a stumbling block. However, exporting things like `ChartType`, `is_chart_type`, `AltairChart` could be a way to expose these shared behaviours - without requiring any changes to the core API.
`TopLevelMixin.save` calls a function accepting `pathlib` objects, but the two didn't share the same `Union` for `fp`. When using `pyright`, the following warning is presented: > Argument of type "Path" cannot be assigned to parameter "fp" of type "str | IO[Unknown]" in function "save" > Type "Path" is incompatible with type "str | IO[Unknown]" > "Path" is incompatible with "str" > "Path" is incompatible with "IO[Unknown]" This fix avoids the need to use type ignores on the public facing API, as a user.
By normalizing `fp` in `save`, it simplifies the signatures of two other functions and removes the need for `str` specific suffix handling.
…sion Although there are branches it isn't needed, I think this is easier to read and maintain. Very low priority change, just something I noticed while reading.
I'm unsure if I've followed the contributing guide correctly. I expected running this locally, without errors, would be safe to push:
I'll hold of on any more commits for now, please let me know if I've missed something |
Thanks for the PR! Without giving a detailed review I can say that the CI errors originate from the auto-generated code. hatch run python tools/generate_schema_wrapper.py See also: https://github.com/vega/altair/blob/main/NOTES_FOR_MAINTAINERS.md#auto-generating-the-python-code. |
Thanks, will give this a try. |
Believe these should be public, but it seems like the issue at build-time is that I'd be extending the public API.
Looks like the same as error as #3418 |
@mattijn still getting the same error, not Is there anything I can do to help here? |
Cleaning the logs and re-running all jobs make all tests pass. This PR is ready for review. @binste has been working hard in the last year to make Altair a typed library, so I hope he can find some time in coming days/week. |
Thanks @dangotbanned! I'll try to have a detailed look at this soon. |
…e duplication of list of charts
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 again for the PR! Agree that this is a helpful utility to have in Altair.
I've made ChartType
and is_chart_type
public and bumped typing_extensions so it includes TypeIs
. I've added some other smaller comments after which I think this is good to go.
As [requested](https://github.com/vega/altair/pull/3420/files/a4de6d1feb6f3f17c7f6d4c7738eb008ff33d17e#r1606780541). Seems to only occur in a few places, but all were autofix-able.
An earlier commit assumed a minor optimization would be possible, by normalizing a `Union` early and reuse the narrowed type in "private" functions. @binste [comment](https://github.com/vega/altair/pull/3420/files#r1610440552)
ChartType
and guard is_chart_type
ChartType
type and type guard is_chart_type
ChartType
type and type guard is_chart_type
ChartType
type and type guard is_chart_type
. Change PurePath to Path type hints
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 again! Looks good to me.
This PR mostly relates to type hints and associated tools, from the perspective of a downstream library author.
Note
Below was automatically added, but is only from the first commit.
The others also have descriptions, not sure if you'd like them all collected into the PR as well.
However, it was the motivation behind the contributions generally so I've left it in.
First commit description
I'm working on a library that uses
altair
, and looking to make it an optional dependency. During this process, I thoughtChartType
andis_chart_type
might be a good fit inaltair
itself.I don't believe any existing
altair
code would directly benefit from these additions - but you could likely reduce someisinstance
checks with type narrowing functions likeis_chart_type
.For example
altair/vegalite/v5/api.py
has 63 occurrences ofisinstance
.per CONTRIBUTING.md
Towards this aim, it could be beneficial to provide
typing
constructs/tools for downstream use.Regarding my specfic use case, the below Protocol - I believe - describes the concept of a
_/Chart
, for the purposes of writing to file:Due to the number of symbols in
altair
and heavy use of mixins, coming to this conclusion can be a stumbling block.However, exporting things like
ChartType
,is_chart_type
,AltairChart
could be a way to expose these shared behaviours - without requiring any changes to the core API.I'd likely have written this differently if
altair
was already using PEP 563, but imagine introducing that could impact the autogenerated code.Did want to note it though, as it might help with some circular references.