Skip to content

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

Closed
1 of 1 issue completed

Description

What is your suggestion?

The following related PRs have been merged since v5.4.1 release:

This issue is the continuation of the discussion below:

Originally posted by @dangotbanned in post review comment

...
In #3536 (comment) I mentioned some related issues, but can now see I forgot to elaborate on what I would prefer for v6.

I would like to have used altair.theme as the namespace instead of altair.typing.theme.
However, this would be very easy to mistake for altair.themes - which is a variable storing a registry of themes.

For v6 I would propose the following api changes - and if there is any support I will open a new issue:

  • altair.typing.theme -> altair.theme
    • Including dropping altair.typing.ThemeConfig, which is already included in the above
  • @altair.register_theme -> @altair.theme.register
  • New function: altair.theme.enable
  • Remove altair.themes top-level export

This would consolidate all theme related functionality behind a single namespace:

from altair import theme

@theme.register("custom 1", enable=False)
def custom_theme() -> theme.ThemeConfig:
    return {
        "autosize": {"contains": "content", "resize": True}, 
        "config": theme.ConfigKwds(
            circle=theme.MarkConfigKwds(fill="purple")
        ),
    }

...

theme.enable("custom 1")
...
theme.enable("default")
Response by @mattijn in comment

...
I like the suggestion you provide in #3536 (comment) to consolidate everything under a single namespace, including the proposed ThemeConfig.
I’m fine with doing that now, rather than releasing a version first and requiring deprecation immediately after.

However, instead of phasing out the plural altair.themes in favour of the singleton altair.theme can we consolidate this PR's work into the existing altair.themes?
For example, can we have altair.themes.ThemeConfig (and altair.themes.AxisConfigKwds, etc.) aligning with the existing altair.themes.register() and altair.themes.enable().

Aesthetically I like the singleton altair.theme, but I think sticking with altair.themes is more practical, if that is also an option. Can you reflect on this?

Proposing this issue itself


Also tagging @binste, as our discussion in another issue probably needs consideration


Notes

  • alt.vegalite.v5.theme, alt.utils.theme were not mentioned in the discussion - stating them here for completeness

Edit

@mattijn apologies for the delay, but see #3610 (comment) for my thoughts

Sub-issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions