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

infer argument types from vegalite schema #2854

Closed
mattijn opened this issue Jan 25, 2023 · 2 comments · Fixed by #3208
Closed

infer argument types from vegalite schema #2854

mattijn opened this issue Jan 25, 2023 · 2 comments · Fixed by #3208

Comments

@mattijn
Copy link
Contributor

mattijn commented Jan 25, 2023

Based on these two comments in this PR #2846:

Originally posted by @mattijn in #2846 (comment)

I understand that adding another dependency just to reduce 5 lines of code is probably not worth it. I also agree that there may be other recent type-related changes in the core of Python that makes it something to consider if we look a bit broader than this PR alone.

We previously had two types of discussions about types in Altair.

Especially the result of the second bullet has set the type Any to anything that is Undefined finishing this discussion on, 'what is the type of Undefined?'.

However, with your impressive PRs regarding jsonschema and types, I was wondering, since types are included in the vega-lite jsonschema, if these types can also be inferred for each argument? Maybe recent type-related changes in the core of Python can make this easier? It is merely an open question from which I don't know enough if it is worthwhile to consider.

and

Originally posted by @binste in #2846 (comment)

Thanks for writing this down! This is very helpful. It should be possible to create Python type hints based on the vl schema, especially given that this parsing is already done for the docstrings, see e.g. https://github.com/altair-viz/altair/blob/master/altair/vegalite/v5/schema/channels.py#L12509 where the stack argument to alt.X is documented as:

stack : anyOf(:class:`StackOffset`, None, boolean)

There are probably also synergies with replacing the vega-lite types with Python types in all error messages (suggested by you in #2842 (comment)) and docstrings.

As more type hints shouldn't break anyones code, I don't think we would need to necessarily include it in the release candidate but I'm definitely intrigued in giving this a try. I really like the development experience you get for a library with well-defined type hints such as better autocompletion as well as early warnings if you pass a wrong value to an argument before even executing the code, for example if you'd pass an integer to stack. The same linters could then of course be used in CI/CD pipelines.

I'll try to get a draft PR going in the coming weeks so we can further explore how well it works.

@mattijn
Copy link
Contributor Author

mattijn commented Feb 11, 2023

Type checking from the jsonschema is interesting, but it is likely that the flexibility of python does not always match with the schema. See e.g. #2877

@binste
Copy link
Contributor

binste commented Mar 12, 2023

For reference, the Pydantic docs have a mapping of Python types to json schema types: https://docs.pydantic.dev/1.10/usage/schema/#json-schema-types

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

Successfully merging a pull request may close this issue.

2 participants