refactor: `alt.themes` -> `alt.theme.themes` #3618

dangotbanned · 2024-09-28T13:52:41Z

Description

This PR aims to consolidate theme related functionality behind a single namespace.
Plans & discussion have been collected in:

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

Consolidate: alt.theme(s), alt.typing.theme, alt.vegalite.v5.theme, alt.utils.theme #3610 (comment)

Tasks

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`

altair/vegalite/v5/__init__.py

#3618 (comment)

Was dependent on `v5` having the registry exported in `__init__`

#3618 (comment)

doc/user_guide/customization.rst

tools/generate_api_docs.py

tools/generate_schema_wrapper.py

altair/vegalite/v5/theme.py

altair/theme.py

altair/__init__.py

dangotbanned · 2024-09-29T19:21:12Z

altair/__init__.py

+def __getattr__(name: str) -> _Any:
+    from altair.utils.deprecation import deprecated_warn
+
+    if name == "themes":


Solution is fine currently, but as soon as we start deprecating other imports it would make sense to generalize

dangotbanned · 2024-09-29T19:23:10Z

altair/__init__.py

+            "https://altair-viz.github.io/user_guide/customization.html#chart-themes",
+            version="5.5.0",
+            alternative="altair.theme.themes",
+            stacklevel=3,


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

#3618 (comment)

https://www.sphinx-doc.org/en/master/usage/domains/python.html#cross-referencing-python-objects

dangotbanned · 2024-09-29T21:10:16Z

altair/vegalite/v5/schema/_config.py

@@ -93,7 +93,7 @@

 class AreaConfigKwds(TypedDict, total=False):
    """
-    :class:`AreaConfig` ``TypedDict`` wrapper.
+    :class:`altair.AreaConfig` ``TypedDict`` wrapper.


Previously these didn't work as links in the API reference.

Now they do

Note

I expect this hasn't come up before since everything has already been in altair.___.

The same will apply for any symbols in altair.(theme|typing)

dangotbanned · 2024-09-29T21:38:27Z

altair/theme.py

+enable = themes.enable
+get = themes.get
+names = themes.names


Note

Originally planned for this to be a temporary solution.

Quite surpised at how well aliasing worked:

Docstring

API Reference

Indistinguishable from an actual function

- Require fewer parameters - Remove need for format string - Adds general `path_to_module_str` utility #3618 (comment)

The rest is still different enough that I'm not planning to further generalize between the two approaches

https://github.com/vega/altair/actions/runs/11108757298/job/30862467326?pr=3618

https://github.com/vega/altair/actions/runs/11109202680/job/30863988787?pr=3618

I've paired these with the `toctree` name, except for `api`. Since that would be `api-api`

dangotbanned · 2024-09-30T17:59:37Z

doc/user_guide/api.rst

+.. _api-theme:
+
+Theme
+-----
+.. currentmodule:: altair.theme
+
+.. autosummary::
+   :toctree: generated/theme/
+   :nosignatures:
+
+   enable
+   get
+   names
+   register
+   themes
+   unregister
+   ThemeConfig
+   AreaConfigKwds
+   AutoSizeParamsKwds


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

a9538bc

a9538bc (#3618)

58da996

58da996 (#3618)

Updated following 3e6bde8 (#3618)

#3607

dangotbanned · 2024-09-30T20:06:26Z

doc/user_guide/customization.rst

+.. note::
+
+   This material was changed considerably with the release of Altair ``5.5.0``.


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)

#3618 (comment)

I added this while experimenting with a more complex `__getattr__`, but don't need it for anything in this version

dangotbanned · 2024-10-01T09:52:21Z

altair/__init__.py

+        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",


alt.themes deprecation warning message

#3610 (comment)

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

dangotbanned · 2024-10-01T13:00:19Z

altair/theme.py

+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)


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

chore: Add notes for change targets

54e7e48

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`

dangotbanned added maintenance deprecation Requires a **MINOR** version bump labels Sep 28, 2024

dangotbanned added 3 commits September 28, 2024 15:15

chore: Add TODO in update_init_file.py

66c51c3

refactor: Remove altair.vegalite.v5.theme from alt.__all__

754f665

refactor: Add alt.theme.py

3dd9100

dangotbanned commented Sep 29, 2024

View reviewed changes

altair/vegalite/v5/__init__.py Outdated Show resolved Hide resolved

dangotbanned mentioned this pull request Sep 29, 2024

Reduce the number of entries available in the top level API #2918

Open

dangotbanned added 9 commits September 29, 2024 13:22

build(typing): Generate altair.vegalite.v5.schema.__init__.__all__

b1be633

#3618 (comment)

build: Generate altair.vegalite.v5.__init__.__all__

0bd8f75

#3618 (comment)

refactor: utils.theme -> vegalite.v5.theme

c996c95

refactor: Remove alt.typing.theme

c286c38

refactor: Adds alt.theme implementation

ef9b846

test: Fix test_common

e8d3062

Was dependent on `v5` having the registry exported in `__init__`

chore(typing): Remove now fix ignore comments

f7fe38b

#3618 (comment)

fix(typing): Use a bounded TypeVar to support list[str]

0a1c8d3

refactor: Add deprecation handling alt.__init__.__getattr__

05eca89

dangotbanned commented Sep 29, 2024

View reviewed changes

doc/user_guide/customization.rst Outdated Show resolved Hide resolved

dangotbanned commented Sep 29, 2024

View reviewed changes

tools/generate_api_docs.py Outdated Show resolved Hide resolved

dangotbanned commented Sep 29, 2024

View reviewed changes

tools/generate_schema_wrapper.py Outdated Show resolved Hide resolved

dangotbanned commented Sep 29, 2024

View reviewed changes

altair/vegalite/v5/theme.py Outdated Show resolved Hide resolved

dangotbanned commented Sep 29, 2024

View reviewed changes

altair/theme.py Show resolved Hide resolved

dangotbanned commented Sep 29, 2024

View reviewed changes

altair/__init__.py Outdated Show resolved Hide resolved

dangotbanned commented Sep 29, 2024

View reviewed changes

dangotbanned added 2 commits September 29, 2024 20:51

docs: Adds alt.theme to API Reference

217d7fb

#3618 (comment)

docs: Add missing altair qualifier for TypedDict(s)

5078f88

https://www.sphinx-doc.org/en/master/usage/domains/python.html#cross-referencing-python-objects

dangotbanned commented Sep 29, 2024

View reviewed changes

fix(typing): Preserve generics in PluginEnabler

2339116

dangotbanned commented Sep 29, 2024

View reviewed changes

dangotbanned linked an issue Sep 29, 2024 that may be closed by this pull request

Add @alt.theme.register to API Reference #3607

Open

dangotbanned added 6 commits September 30, 2024 15:54

refactor: Refine and fully document generate_schema__init__

48e44c0

- Require fewer parameters - Remove need for format string - Adds general `path_to_module_str` utility #3618 (comment)

refactor: Use new functions for part of update__all__variable

a7dbaca

The rest is still different enough that I'm not planning to further generalize between the two approaches

fix: Don't consider suffix a part in path_to_module_str

a0947f8

https://github.com/vega/altair/actions/runs/11108757298/job/30862467326?pr=3618

fix: Account for GH runner path

1e9f639

https://github.com/vega/altair/actions/runs/11109202680/job/30863988787?pr=3618

docs: Add link targets for API Reference sections

d895447

I've paired these with the `toctree` name, except for `api`. Since that would be `api-api`

docs: Customize theme toctree order

a9538bc

dangotbanned commented Sep 30, 2024

View reviewed changes

docs: Update User Guide section

e114493

#3607

dangotbanned commented Sep 30, 2024

View reviewed changes

dangotbanned changed the title ~~refactor(DRAFT): alt.themes -> alt.theme.themes~~ refactor: alt.themes -> alt.theme.themes Sep 30, 2024

docs: Provide API ref link in warning

c8033b2

#3618 (comment)

dangotbanned marked this pull request as ready for review September 30, 2024 20:18

dangotbanned requested a review from mattijn September 30, 2024 20:19

refactor: Remove unused __future__ import

ef0f263

I added this while experimenting with a more complex `__getattr__`, but don't need it for anything in this version

dangotbanned requested a review from binste September 30, 2024 20:25

mattijn mentioned this pull request Oct 1, 2024

Consolidate: alt.theme(s), alt.typing.theme, alt.vegalite.v5.theme, alt.utils.theme #3610

Open

dangotbanned commented Oct 1, 2024

View reviewed changes

dangotbanned added 3 commits October 1, 2024 11:38

feat: Support alt.theme.(active|options)

58da996

#3610 (comment)

fix(typing): Partial resolve mypy __getattr__

bc507a9

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

test: Rename test to test_theme_register_decorator

e0f4721

Now reflects the updated function name

dangotbanned commented Oct 1, 2024

View reviewed changes

dangotbanned mentioned this pull request Oct 1, 2024

PluginRegistry persists unregistered plugins #3619

Open

dangotbanned added 4 commits October 1, 2024 14:23

test: Add some more (theme|themes) equal checks

4e8def0

test: Add test_theme_unregister

9bd0316

The commented out assertion may be added later after resolving #3619

feat: Raise instead of returning None in unregister

0d95b44

docs: Update remaining ThemeRegistry methods

3e6bde8

dangotbanned mentioned this pull request Oct 1, 2024

ci: Use breaking, deprecation labels in changelog #3623

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: `alt.themes` -> `alt.theme.themes` #3618

refactor: `alt.themes` -> `alt.theme.themes` #3618

dangotbanned commented Sep 28, 2024 •

edited

Loading

dangotbanned Sep 29, 2024

dangotbanned Sep 29, 2024

dangotbanned Sep 29, 2024 •

edited

Loading

dangotbanned Sep 29, 2024

dangotbanned Sep 30, 2024 •

edited

Loading

dangotbanned Sep 30, 2024

dangotbanned Oct 1, 2024

dangotbanned Oct 1, 2024

		.. note::

		This material was changed considerably with the release of Altair ``5.5.0``.

refactor: alt.themes -> alt.theme.themes #3618

Are you sure you want to change the base?

refactor: alt.themes -> alt.theme.themes #3618

Conversation

dangotbanned commented Sep 28, 2024 • edited Loading

Description

Implementation

Functional Changes

Tasks

dangotbanned Sep 29, 2024

Choose a reason for hiding this comment

dangotbanned Sep 29, 2024

Choose a reason for hiding this comment

dangotbanned Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Note

dangotbanned Sep 29, 2024

Choose a reason for hiding this comment

Note

dangotbanned Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Preview

Earlier versions

Updated following 3e6bde8 (#3618)

dangotbanned Sep 30, 2024

Choose a reason for hiding this comment

Note

dangotbanned Oct 1, 2024

Choose a reason for hiding this comment

dangotbanned Oct 1, 2024

Choose a reason for hiding this comment

Note

refactor: `alt.themes` -> `alt.theme.themes` #3618

refactor: `alt.themes` -> `alt.theme.themes` #3618

dangotbanned commented Sep 28, 2024 •

edited

Loading

dangotbanned Sep 29, 2024 •

edited

Loading

dangotbanned Sep 30, 2024 •

edited

Loading

Updated following `3e6bde8` (#3618)