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.

Sign up for GitHub

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(typing): Adds public altair.typing module #3515

Merged
merged 36 commits into from
Aug 8, 2024
Merged

feat(typing): Adds public altair.typing module #3515

Changes from 1 commit
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
680b69e
feat(typing): Create `altair.typing`
dangotbanned Aug 3, 2024
785f89c
chore: Add comment on `tooltip` annotation
dangotbanned Aug 3, 2024
5423748
feat(typing): Add reference impl `EncodeKwds` from comment
dangotbanned Aug 3, 2024
da64738
feat(typing): Use `OneOrSeq[tps]` instead of `Union[*tps, list]` in `…
dangotbanned Aug 3, 2024
5877b38
build: run `generate-schema-wrapper`
dangotbanned Aug 3, 2024
4fe5f3a
wip: generate `typed_dict_args`
dangotbanned Aug 3, 2024
0014cc8
refactor: Simplify `tools`, remove redundant code
dangotbanned Aug 3, 2024
226038d
refactor: finish removing `altair_classes_prefix`
dangotbanned Aug 3, 2024
77b101a
feat: `_create_encode_signature()` -> `EncodingArtifacts`
dangotbanned Aug 3, 2024
675bc4e
build: run `generate-schema-wrapper`
dangotbanned Aug 3, 2024
51a84a5
feat(typing): Provide a public export for `_EncodeKwds`
dangotbanned Aug 3, 2024
2ef4b0f
Merge branch 'main' into public-typing
dangotbanned Aug 3, 2024
4e0a098
Merge branch 'main' into pr/dangotbanned/3515
binste Aug 4, 2024
2b9ad2c
Add docstring to _EncodeKwds
binste Aug 4, 2024
79f317d
Rewrite EncodeArtifacts dataclass as a function
binste Aug 4, 2024
1eb466d
Fix ruff issue due to old local ruff version
binste Aug 4, 2024
0287eba
Change generate_encoding_artifacts to an iterator
binste Aug 4, 2024
bac1f67
docs: run `generate-schema-wrapper` with `indent_level=4`
dangotbanned Aug 4, 2024
3419250
feat(typing): Move `ChartType`, `is_chart_type` to `alt.typing`
dangotbanned Aug 4, 2024
5321b4b
Merge remote-tracking branch 'upstream/main' into public-typing
dangotbanned Aug 4, 2024
d16ec34
revert(ruff): Restore original ('RUF001`) line
dangotbanned Aug 4, 2024
e903528
Add type aliases for each channel
binste Aug 5, 2024
6662fc9
Format
binste Aug 5, 2024
28de27b
Use Union instead of | for compatibility with Py <3.10
binste Aug 5, 2024
b3fbe9c
Add channel type aliases to typing module. Add 'Type hints' section t…
binste Aug 6, 2024
5ba8a8d
chore(ruff): Remove unused `F401` ignore
dangotbanned Aug 6, 2024
49122b1
feat(typing): Move `Optional` export to `typing`
dangotbanned Aug 6, 2024
fe22c80
refactor: Move blank line append to `indent_docstring`
dangotbanned Aug 6, 2024
d3daf51
docs(typing): Remove empty type list from `EncodeKwds`
dangotbanned Aug 6, 2024
914428a
refactor: Renaming, grouping, reducing repetition
dangotbanned Aug 6, 2024
11c58c3
refactor: More tidying up, annotating, reformat
dangotbanned Aug 6, 2024
067f455
docs: Reference aliases in `generate_encoding_artifacts`
dangotbanned Aug 6, 2024
6fefd12
Use full type hints instead of type alias in signatures for typeddict…
binste Aug 7, 2024
9299a81
Merge remote-tracking branch 'upstream/main' into public-typing
dangotbanned Aug 7, 2024
b6f84e4
Rename 'Type hints' to 'Typing'
binste Aug 8, 2024
d4313c0
Ruff fix
binste Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 2 additions & 60 deletions altair/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,63 +1,5 @@
from __future__ import annotations

import sys
from typing import Any, Mapping, Union
from typing_extensions import TypedDict
__all__ = ["EncodeKwds"]

if sys.version_info >= (3, 10):
from typing import TypeAlias
else:
from typing_extensions import TypeAlias

ChannelType: TypeAlias = Union[str, Mapping[str, Any], Any]
Copy link
Member Author

@dangotbanned dangotbanned Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@binste I know I removed ChannelType here, but altair.typing could define the precise types for each of the positional-args.

pola-rs/polars#17995 (comment)

image

E.g. ChannelX, ChannelY, ..., or similar?

These did vary between each method, but I think this would be 6 unique channel type aliases total:

Unsure if there would be more needed for the yet-unwritten methods

Copy link
Contributor

Choose a reason for hiding this comment

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

You already got very far! Great to see that we're almost there 😄

I'm going through it now but I'm also wondering if we maybe just want to specify, for each channel, the types separately. ChannelX, ChannelY, etc. works for me regarding naming. So basically what's there now for EncodingKwgs but as individual type aliases.

I don't know if we then even need a TypedDict, what do you think? Unpack supports the usage suggested in pola-rs/polars#17995 (comment) only for Python >= 3.12. And I think the plot methods should then only accept **kwargs: Unpack[EncodeKwgs] as else the arguments which are explicitly defined are defined twice. See typing docs.

Downside would be that in Polars, users only get autocomplete and type checking for the channels which are explicitly defined but for simple exploratory charts, this could be enough?

Copy link
Member Author

@dangotbanned dangotbanned Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You already got very far! Great to see that we're almost there 😄

Thanks @binste for such a thorough response!

And I think the plot methods should then only accept **kwargs: Unpack[EncodeKwds] as else the arguments which are explicitly defined are defined twice. See typing docs.

I was trying to demonstrate the correct way to use Unpack there, to avoid that issue (the second one):

def foo(name, **kwargs: Unpack[Movie]) -> None: ...     # WRONG! "name" will always bind to the first parameter.

def foo(name, /, **kwargs: Unpack[Movie]) -> None: ...  # OK! "name" is a positional-only parameter,
           # ^^^                                        # so **kwargs can contain a "name" keyword.
Long screenshot

image

I don't know if we then even need a TypedDict, what do you think? Unpack supports the usage suggested in pola-rs/polars#17995 (comment) only for Python >= 3.12.

I'm not sure there's an issue with typing_extensions.Unpack which I used unconditionally there?
Usually ruff or pylance will warn me when I do something that isn't allowed in py3.8

image

@binste

Downside would be that in Polars, users only get autocomplete and type checking for the channels which are explicitly defined but for simple exploratory charts, this could be enough?

@dangotbanned comment

You could then repeat the /, **kwargs: Unpack[EncodeKwds] for the other methods - maybe changing the positional-only ones if needed

This was essentially the compromise of that, if that makes sense?
You have your common ones that can be called without kwargs, but more options with kwargs

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're absolutely right! Thanks for the detailed explanation. Lost a bit the overview there but this brought me back on track :)

Just tested /, **kwargs: Unpack[EncodeKwds] with typing_extensions.Unpack on Python 3.11 and works as expected.

Reviewing the rest now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MarcoGorelli when you see this

The need for this solution is that alt.Chart.encode has alphabetically ordered parameters, whereas pl.DataFrame.plot has:

  • pos-only (in a different order)
  • *args
  • **kwargs

Copy link
Member Author

Choose a reason for hiding this comment

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

Marking as unresolved purely so this is easier to find during review

It is actually resolved unless #3515 (comment) turns up anything new



class EncodeKwds(TypedDict, total=False):
"""
Reference implementation from [comment code block](https://github.com/pola-rs/polars/pull/17995#issuecomment-2263609625).

Aiming to define more specific `ChannelType`, without being exact.

This would be generated alongside `tools.generate_schema_wrapper._create_encode_signature`
"""

angle: ChannelType
color: ChannelType
column: ChannelType
description: ChannelType
detail: ChannelType | list[Any]
facet: ChannelType
fill: ChannelType
fillOpacity: ChannelType
href: ChannelType
key: ChannelType
latitude: ChannelType
latitude2: ChannelType
longitude: ChannelType
longitude2: ChannelType
opacity: ChannelType
order: ChannelType | list[Any]
radius: ChannelType
radius2: ChannelType
row: ChannelType
shape: ChannelType
size: ChannelType
stroke: ChannelType
strokeDash: ChannelType
strokeOpacity: ChannelType
strokeWidth: ChannelType
text: ChannelType
theta: ChannelType
theta2: ChannelType
tooltip: ChannelType | list[Any]
url: ChannelType
x: ChannelType
x2: ChannelType
xError: ChannelType
xError2: ChannelType
xOffset: ChannelType
y: ChannelType
y2: ChannelType
yError: ChannelType
yError2: ChannelType
yOffset: ChannelType
from altair.vegalite.v5.schema.channels import _EncodeKwds as EncodeKwds