Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Adds 4 missing carbon themes, provide autocomplete #3516

Merged
merged 6 commits into from
Aug 4, 2024
Merged

feat: Adds 4 missing carbon themes, provide autocomplete #3516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a529481
style: Sort `VEGA_THEMES` alphabetically
dangotbanned Aug 3, 2024
8e2990b
feat: Add missing `carbon...` themes
dangotbanned Aug 3, 2024
dca03b9
feat(typing): Support autocomplete for `themes.enable(name)`
dangotbanned Aug 3, 2024
42fac3b
chore: Fix `PluginRegistery` comment typo
dangotbanned Aug 3, 2024
105aab6
Merge branch 'main' into update-vega-themes
dangotbanned Aug 3, 2024
2f08c63
Add comments
binste Aug 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions altair/utils/theme.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,49 @@
"""Utilities for registering and working with themes."""

from typing import Callable
from __future__ import annotations

import sys
from typing import TYPE_CHECKING, Callable

from .plugin_registry import PluginRegistry

if sys.version_info >= (3, 11):
from typing import LiteralString
else:
from typing_extensions import LiteralString

if TYPE_CHECKING:
from altair.utils.plugin_registry import PluginEnabler
from altair.vegalite.v5.theme import _ThemeName

ThemeType = Callable[..., dict]


class ThemeRegistry(PluginRegistry[ThemeType, dict]):
pass
def enable(
self, name: LiteralString | _ThemeName | None = None, **options
) -> PluginEnabler:
"""
Enable a theme by name.

This can be either called directly, or used as a context manager.

Parameters
----------
name : string (optional)
The name of the theme to enable. If not specified, then use the
current active name.
**options :
Any additional parameters will be passed to the theme as keyword
arguments

Returns
-------
PluginEnabler:
An object that allows enable() to be used as a context manager

Notes
-----
Default `vega` themes can be previewed at https://vega.github.io/vega-themes/
"""
return super().enable(name, **options)
49 changes: 41 additions & 8 deletions altair/vegalite/v5/theme.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,54 @@

from __future__ import annotations

from typing import Final
from typing import TYPE_CHECKING, Final, Literal

from altair.utils.theme import ThemeRegistry

if TYPE_CHECKING:
import sys

if sys.version_info >= (3, 10):
from typing import TypeAlias
else:
from typing_extensions import TypeAlias

# If you add a theme here, also add it in `VEGA_THEMES` below.
_ThemeName: TypeAlias = Literal[
"default",
"carbonwhite",
"carbong10",
"carbong90",
"carbong100",
"dark",
"excel",
"fivethirtyeight",
"ggplot2",
"googlecharts",
"latimes",
"opaque",
"powerbi",
"quartz",
"urbaninstitute",
"vox",
]

# If you add a theme here, also add it in `_ThemeName` above.
VEGA_THEMES = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of defining the theme names twice, I think you could specify _ThemeName as you've done here and then VEGA_THEMES could become typing.get_args(_ThemeName). See https://docs.python.org/3/library/typing.html#typing.get_args

Copy link
Member Author

@dangotbanned dangotbanned Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @binste for the review!

I think I could rework this to use typing.get_args, but wanted to note some things first that led to my duplication here.

I'm viewing _ThemeName as a short-term solution:

  • Explicitly marking it "private"
  • Defining within a TYPE_CHECKING block to prevent it existing at runtime.

I don't think I've used this pattern anywhere in altair before, but some examples in other projects (for their own reasons):

Examples

IIRC, I'd need to move _ThemeName out of that block or use typing.get_type_hints

I'd also need to remove "default", "opaque" in the def of _ThemeName or later during iteration:

Registration source

altair/altair/vegalite/v5/theme.py

Lines 45 to 61 in c12ca27

themes.register(
"default",
lambda: {"config": {"view": {"continuousWidth": 300, "continuousHeight": 300}}},
)
themes.register(
"opaque",
lambda: {
"config": {
"background": "white",
"view": {"continuousWidth": 300, "continuousHeight": 300},
}
},
)
themes.register("none", dict)
for theme in VEGA_THEMES:
themes.register(theme, VegaTheme(theme))

For a long-term solution, I'd want to:

  • Extract the names directly from source as (VegaThemes: Literal[...])
  • Define altair themes as (AltairThemes: Literal["opaque", ...)
  • _ThemeName -> DefaultThemes: TypeAlias = AltairThemes | VegaThemes
  • Annotate themes.enable(name: DefaultThemes)
  • Revisit the need for defining this across two theme.py modules, as part of Flatten the package structure #3337

Essentially I want to keep this as flexible as possible for now.

Planning to open another issue to discuss themes more generally.
There are other ideas I've had and I got the impression you might also have some of your own 🤝

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steps re long-term solution sound great! We could probably keep it simple and just fetch the themes from the latest source file in vega-themes which you linked. Or the file which was part of the latest release.

Let's continue the discussion in a new issue as you suggested, thanks for bringing it up! :) I have some notes on providing a better default theme and additional documentation around customization (advanced legend manipulation, vertical y-axis titles, y-axis on the right side with tick labels on top of the ticks, ...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added 2 comments to mention that these two variables need to be kept in sync for now. Just in case we don't get to an improved solution for a while.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steps re long-term solution sound great! We could probably keep it simple and just fetch the themes from the latest source file in vega-themes which you linked. Or the file which was part of the latest release.

Let's continue the discussion in a new issue as you suggested, thanks for bringing it up! :) I have some notes on providing a better default theme and additional documentation around customization (advanced legend manipulation, vertical y-axis titles, y-axis on the right side with tick labels on top of the ticks, ...)

Perfect, thanks again @binste

"ggplot2",
"quartz",
"vox",
"fivethirtyeight",
"carbonwhite",
"carbong10",
"carbong90",
"carbong100",
"dark",
"latimes",
"urbaninstitute",
"excel",
"fivethirtyeight",
"ggplot2",
"googlecharts",
"latimes",
"powerbi",
"quartz",
"urbaninstitute",
"vox",
]


Expand All @@ -38,7 +71,7 @@ def __repr__(self) -> str:

# The entry point group that can be used by other packages to declare other
# themes that will be auto-detected. Explicit registration is also
# allowed by the PluginRegistery API.
# allowed by the PluginRegistry API.
ENTRY_POINT_GROUP: Final = "altair.vegalite.v5.theme"
themes = ThemeRegistry(entry_point_group=ENTRY_POINT_GROUP)

Expand Down