-
Notifications
You must be signed in to change notification settings - Fork 793
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
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
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! |
@mattijn I'm planning to merge this tomorrow if you don't have any objections. In #3600 I've been trying to work around not having the refactored |
Thanks for asking! I still cannot understand why this Also, as a user, I really prefer a feature explanation that I can copy and paste. I don't see that in this PR. I see a few screenshots, but that is really not the advocated route for a Minimal, Reproducible Example. For me it is still not clear how I can use this from the PR description. I found the following in the code: import altair as alt
from altair.typing import ThemeConfig
from vega_datasets import data
@alt.register_theme("param_font_size", enable=True)
def custom_theme() -> ThemeConfig:
sizes = 12, 14, 16, 18, 20
return {
"autosize": {"contains": "content", "resize": True},
"background": "#F3F2F1",
"config": {
"axisX": {"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
"axisY": {"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
"font": "'Lato', 'Segoe UI', Tahoma, Verdana, sans-serif",
"headerColumn": {"labelFontSize": sizes[1]},
"headerFacet": {"labelFontSize": sizes[1]},
"headerRow": {"labelFontSize": sizes[1]},
"legend": {"labelFontSize": sizes[0], "titleFontSize": sizes[1]},
"text": {"fontSize": sizes[0]},
"title": {"fontSize": sizes[-1]},
},
"height": {"step": 28},
"width": 350,
}
source = data.stocks()
lines = (
alt.Chart(source, title=alt.Title("Stocks"))
.mark_line()
.encode(x="date:T", y="price:Q", color="symbol:N")
)
lines.interactive(bind_y=False) That works! But I also see another approach. Here you define a from 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 How do I register this as theme? I searched for Last, after I enabled the custom theme, how can I de-register this custom theme and go back to the default settings? Thank you for your patience! |
Why
|
Screenshots vs Code blocks
I have now updated the PR description with collapsible code blocks to accompany each screenshot. I would absolutely agree that providing a Minimal, Reproducible Example is helpful, although I thought that related more to bugs/issues? There were a few reasons leaned towards screenshots:
|
@alt.register_theme
I've updated the PR description, this is the example under the heading Options for imports & runtime typing
Well spotted! I was waiting until this PR was finished before introducing There are a few other changes I've noted for the User Guide in #3519, including
Exactly the same way as documented in https://altair-viz.github.io/user_guide/customization.html#defining-a-custom-theme In #3526 I've described this as a convenience for the common case where you use a single theme. |
I didn't mean to trigger a merge to I was trying to fix the conflicts but now I see the comment I'd mentioned was unresolved in #3536 (comment) has been resolved
EditSee #3536 (comment) |
Thanks for the added explanations! Appreciated. I like the suggestion you provide in #3536 (comment) to consolidate everything under a single namespace, including the proposed However, instead of phasing out the plural Aesthetically I like the singleton Thank you for the added explanations regarding the type of Also, much appreciated for including both code blocks and screenshots! Great combination as such! |
I don't think it is possible to re-open a merged PR, even when the changes are reverted with the latest commit, probably the best option is to open a new PR😔 |
Thanks @mattijn
Okay so rather than fully revert this PR, I think the changes we want are strictly concerning a small refactor of the public interface. I propose:
How does that sound to you? |
The changes requested were only concerning a small refactor of the public interface. Proposal sounds good! |
I commented this out in #3536 to identify remaining typing issues. All of `"./tools/**/*.py"` is now typed, so re-enabling in the future is very unlikely
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
typesCode block
Type checking w/ mixed
python|altair
typesCode block
Options for imports & runtime typing
Code block
@register_theme
docDefault
Code block
With a registered theme
Code block
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