-
Notifications
You must be signed in to change notification settings - Fork 794
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
#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__`
The rules here are not the same as the rules for [`LiteralString`](https://typing.readthedocs.io/en/latest/spec/literal.html#literalstring) Hoping to avoid any confusion if anyone uses this later
@mattijn I've addressed (#3618 (comment)) in (a0497e0) All of the Unresolved conversations are just to help make the review easier. |
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.
Thanks, I approve. However, I’d like another maintainer to review this as well due to the significant impact of this PR. Once merged, it will require updates to several published packages on PyPI, such as those listed here: https://pypi.org/search/?q=altair+theme.
Thanks for the review @mattijn! Yeah that makes sense, always good to have another set of eyes |
Apologies if this was already clear, but I just wanted to restate it in a short format for some reassurance.
|
Caught that one quite late 😅 (#3618 (comment)) |
Any opinion on this @joelostblom, @jonmmease? I'm not a themes/theme user myself, so it is hard for me to overview the impact. Do we know anyone who is/are using the theme and will be impacted by this? |
Or, put positively, someone who could benefit from this PR and might be able to provide a quick check. Otherwise I will press the merge button in a week, November 11. |
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)