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

feat(typing): adds Map alias for Mapping[str, Any] #3458

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jul 2, 2024

This allows a TypedDict to be used, where mypy previously would have required exactly a dict.

The name does not conflict with any existing types/classes in altair and has the benefit of being short like dict, but more permissive.

I have only added this to Chart.encode as I know this does not require the mutability of dict, which I cannot confidently say for elsewhere in altair.
A future PR could investigate other generated wrappers that use dict and either reuse Map or add another alias to the EXTRA_ALIASES literal.

Fixes #3427 (comment)

cmd [6] | mypy altair tests
tests\vegalite\v5\test_api.py:375: error: Unused "type: ignore" comment  [unused-ignore]
tests\vegalite\v5\test_api.py:376: error: Unused "type: ignore" comment  [unused-ignore]

Related https://mypy.readthedocs.io/en/stable/typed_dict.html#typeddict

This allows a `TypedDict` to be used, where `mypy` previously would have required exactly a `dict`.

The name is not used anywhere currently in `altair` and has the benefit of being short like `dict`, but more permissive.

I have only added this to `Chart.encode` as I know this does not require the mutability of `dict`, which I cannot confidently say for elsewhere in `altair`.

Fixes vega#3427 (comment)
@binste
Copy link
Contributor

binste commented Jul 2, 2024

Looks good to me, thanks! Agree that it would make sense to relax this type hint also in other places.

@binste binste merged commit 91a1de1 into vega:main Jul 2, 2024
12 checks passed
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jul 2, 2024
@dangotbanned
Copy link
Member Author

Looks good to me, thanks! Agree that it would make sense to relax this type hint also in other places.

thanks @binste

Could you reopen #3427 please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants