-
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
refactor: alt.themes
-> alt.theme.themes
#3618
base: main
Are you sure you want to change the base?
Conversation
Need a way to make sense of these and start GH threads: - 10 total files - 3 modules named `theme.py` - 2 modules named `__init__.py`
Was dependent on `v5` having the registry exported in `__init__`
def __getattr__(name: str) -> _Any: | ||
from altair.utils.deprecation import deprecated_warn | ||
|
||
if name == "themes": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solution is fine currently, but as soon as we start deprecating other imports it would make sense to generalize
"https://altair-viz.github.io/user_guide/customization.html#chart-themes", | ||
version="5.5.0", | ||
alternative="altair.theme.themes", | ||
stacklevel=3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was testing, the default I'd set of 2 didn't report any warnings.
Not sure if that is related to __getattr__
itself, but noting it anyway
@@ -93,7 +93,7 @@ | |||
|
|||
class AreaConfigKwds(TypedDict, total=False): | |||
""" | |||
:class:`AreaConfig` ``TypedDict`` wrapper. | |||
:class:`altair.AreaConfig` ``TypedDict`` wrapper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enable = themes.enable | ||
get = themes.get | ||
names = themes.names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest is still different enough that I'm not planning to further generalize between the two approaches
I've paired these with the `toctree` name, except for `api`. Since that would be `api-api`
doc/user_guide/api.rst
Outdated
.. _api-theme: | ||
|
||
Theme | ||
----- | ||
.. currentmodule:: altair.theme | ||
|
||
.. autosummary:: | ||
:toctree: generated/theme/ | ||
:nosignatures: | ||
|
||
enable | ||
get | ||
names | ||
register | ||
themes | ||
unregister | ||
ThemeConfig | ||
AreaConfigKwds | ||
AutoSizeParamsKwds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preview
I think this order makes the most sense.
- Functions will be the first thing you need
- The top level config is clearly visible
- Then further config via
...Kwds
.
Earlier versions
Updated following 3e6bde8
(#3618)
.. note:: | ||
|
||
This material was changed considerably with the release of Altair ``5.5.0``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Trying to keep this PR focused to make for an easier review.
I am planning to develop this section further (#3519), but would like to do that in follow-up PRs.
(All still prior to v5.5.0
)
alt.themes
-> alt.theme.themes
alt.themes
-> alt.theme.themes
I added this while experimenting with a more complex `__getattr__`, but don't need it for anything in this version
deprecated_warn( | ||
"Most of the `ThemeRegistry` API is accessible via `altair.theme`.\n" | ||
"See the updated User Guide for further details:\n" | ||
" https://altair-viz.github.io/user_guide/api.html#theme\n" | ||
" https://altair-viz.github.io/user_guide/customization.html#chart-themes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alt.themes
deprecation warning message
Didn't have any issues with `pyright`. Adapted from https://stackoverflow.com/a/78369397 https://github.com/vega/altair/actions/runs/11123634307/job/30907482239?pr=3618
Now reflects the updated function name
altair/theme.py
Outdated
def unregister(name: LiteralString) -> Plugin[ThemeConfig] | None: | ||
""" | ||
Remove and return a previously registered theme. | ||
|
||
Parameters | ||
---------- | ||
name | ||
Unique name assigned in ``alt.theme.themes``. | ||
""" | ||
return themes.register(name, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
- More explicit than being part of
themes.register
- This behavior is not part of
@alt.theme.register
- It wouldn't make sense decorating a function with it
The commented out assertion may be added later after resolving #3619
Will close #3610, #3607
Description
This PR aims to consolidate theme related functionality behind a single namespace.
Plans & discussion have been collected in:
alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)Implementation
I would recommend reading #3618 (comment) first, as an unexpected issue arose.
Solving this contributed +1400 additional lines, of which 85% is generated code in (https://github.com/vega/altair/pull/3618/files#diff-3426a297d4278b99a32d85b11c425f5479dab750a63ba4ff77ba75870946cadf), (https://github.com/vega/altair/pull/3618/files#diff-7ed2cae75fa3376b19115773b5d58bd76f12c6e10a0ed7a03b22e098842b7b1f)
Functional Changes
alt.theme(s)
,alt.typing.theme
,alt.vegalite.v5.theme
,alt.utils.theme
#3610 (comment)Tasks
alt.__init__.__getattr__
alt.theme.py
@alt.register_theme
->@alt.theme.register
alt.theme.enable
alt.theme.(active|options)
generate_schema__init__
->48e44c0
(#3618)