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 |
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
binste
left a comment
There was a problem hiding this comment.
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_typeChartType type and type guard is_chart_type
ChartType type and type guard is_chart_typeChartType type and type guard is_chart_type. Change PurePath to Path type hints
binste
left a comment
There was a problem hiding this comment.
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 thoughtChartTypeandis_chart_typemight be a good fit inaltairitself.I don't believe any existing
altaircode would directly benefit from these additions - but you could likely reduce someisinstancechecks with type narrowing functions likeis_chart_type.For example
altair/vegalite/v5/api.pyhas 63 occurrences ofisinstance.per CONTRIBUTING.md
Towards this aim, it could be beneficial to provide
typingconstructs/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
altairand heavy use of mixins, coming to this conclusion can be a stumbling block.However, exporting things like
ChartType,is_chart_type,AltairChartcould be a way to expose these shared behaviours - without requiring any changes to the core API.I'd likely have written this differently if
altairwas 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.