-
Notifications
You must be signed in to change notification settings - Fork 787
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 ThemeConfig
(TypedDict
)
#3536
base: main
Are you sure you want to change the base?
Conversation
Mostly avoiding use `list` unless properties of it are needed. Also using `set` comprehensions
…tion` Provides all existing functionality. Adds `use_concrete`, for upcoming `TypedDict` changes
- Adapted to new `get_python_type_representation` - Avoid nested listcomp - Avoid repeated sorting - Moved the generic parts to `_generate_sig_args`
Will be adding a docstring with more detail
Handled by `jsonschema_to_python_types`
See comments
Thanks so much for the review @binste! (and helping me get unstuck midway through) Hopefully this will make it easier to explore some of your points in #3519 (comment) once we've merged I'll leave this open for a few hours in case you had any thoughts on #3536 (comment) - otherwise I'll follow this up with another PR as mentioned |
Thanks for all the work. A concise PR description with an explanation of the what/why/how would really help future readers coming to this PR. |
No worries, will work on that now thanks @mattijn |
@mattijn just updated the description, hopefully you (and others) find it helpful 😄 |
Thanks! Can you add also how this feature is being used? Do we want users to import the following? import altair.vegalite.v5.schema._config as theme And from altair.vegalite.v5.schema._config import ThemeConfig And then? I'd love to have a brief description that explains how a new user can start using this feature. It would be great if you could provide a simple example that highlights just one feature controlled by the |
Thanks for expanding on the PR description, that was helpful for me to appreciate just how much this PR improves the experience of defining custom themes, wow! In line with what Mattijn suggested, it would be great to update the custom theme section of the docs to use this approach and mention the autocompletion behavior. |
The usage I described in the description was not actually possible: >- The top-level object `ThemeConfig` is exported to `altair.typing`> - All others are accessible via `altair.typing.theme`. This resolves the `ModuleNotFoundError` that would get raised when trying this
Correction (1)from altair.typing import theme Usagefrom altair.typing import ThemeConfig, theme
custom_theme = ThemeConfig(
config=theme.ConfigKwds(
axis=theme.AxisConfigKwds(grid=True, labelFont="system-ui"),
circle=theme.MarkConfigKwds(fill="purple"),
)
)
custom_theme
Correction (2)from altair.typing import ThemeConfig
Reading this made me spot a bug with the import behavior, just fixed in For reference, |
@joelostblom @mattijn thanks for your comments! I'll be updating the description with more examples tomorrow, but hopefully you get a rough idea with
Absolutely agree, I added that to the list in #3519 (comment) an hour ago - I think it will make the remaining items much easier |
Awesome! Looking forward to! Another question I have, why is this |
For I can't seem to find the thread, but I suggested to @binste having a small number of With the current layout, I think this reinforces the relationship between I'm not sure if all that made sense, if not apologies, I was supposed to call it a day already 😅 |
FYI the conversation marked as unresolved is actually resolved. I haven't updated the status of it yet, to block merging until you've both seen the updated description Otherwise this should be ready to merge 👍 |
I don't have much to comment on the changes to the import, so will refer to @mattijn for that. On first look, the class based syntax looks maybe a bit verbose: So I would probably opt for the combination with dicts myself: But maybe I will change my mind when I try them; it's great that both exist and support autocompletion! |
Resolves one item in #3519
Description
This PR aims to improve the UX when authoring a theme.
By utilising
TypedDict
(s) we can provide a very similar experience to that of writing in Vega Editor, with rich autocompletion and static validation feedback.Main Additions
TypedDict
classes, mirroringSchemaBase
classes - for the subset of objects that are used within a theme._.config.py
where these classes reside.tools/
(w/ improved documentation) to support both existing schema generation and these newTypedDict
(s):Interactions with public API
These classes are entirely isolated from the main
alt.___
namespace, but can still be used anywhere adict|Map
annotation is present in existing code.Currently:
ThemeConfig
is exported toaltair.typing
altair.typing.theme
.Using the classes themselves is entirely optional, as can be seen in
test_theme.py
.Simply annotating
ThemeConfig
(and using a static type checker) allows the user to specify a theme using nativepython
types.Examples
Autocomplete w/ native
python
typesType checking w/ mixed
python|altair
typesOptions for imports
@register_theme
docDefault
With a registered theme
Early POC Demo
These screenshots were taken mid-development
Providing fully nested auto-complete
Type checking feedback
Tasks
ThemeConfig
"parent`ThemeConfig
(TypedDict
) #3536 (comment)ThemeConfig
(TypedDict
) #3536 (commits)EncodeKwds
, where docs do not contain typing informationThemeConfig
TypedDict
,Literal
, or a composition that contains to 0SchemaBase
typesbb99389
(#3536)MANUAL_DEFS
w/ a recursive/graph-based solution3c9aab5
(#3536) to better understand the surrounding code