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.

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

feat: Adds vega-themes.json using vl_convert #3523

Merged
merged 12 commits into from
Aug 8, 2024

Conversation

dangotbanned
Copy link
Member

#3519 (comment)

Provides inline definitions from vega-themes.

Thought this made more sense as .json, rather than a dict within a module.

Eventually these could each be read into a Theme/Config TypedDict as mentioned in #3519

@dangotbanned
Copy link
Member Author

@jonmmease Seem to have resolved in 4c3b189 (#3523) by pruning my envs.

I don't quite understand how my vl_convert version wouldn't have included the newer themes #3523 (comment), but all good now

@dangotbanned
Copy link
Member Author

dangotbanned commented Aug 7, 2024

@jonmmease we could keep this PR atomic and just merge as-is.

I did some playing around with alt.Config yesterday, but I think it might be some time before I've got a clean solution to recursively build a TypedDict and (str|float|list) equivalent hierarchy.

What do you think?

@dangotbanned dangotbanned marked this pull request as ready for review August 7, 2024 10:25
@jonmmease
Copy link
Contributor

Yeah, it's not a very large file, so I think it's fine to merge without being used. And I think someone could technically load the json file directly using importlib.resources if they really wanted to.

Let's leave it open another day to see if there are any other opinions. Feel free to ping me in 24 hrs if we haven't heard anything and I'll merge it.

@dangotbanned
Copy link
Member Author

dangotbanned commented Aug 7, 2024

Sounds good to me @jonmmease

And I think someone could technically load the json file directly using importlib.resources if they really wanted to.

This was the shortest I came up with:

diff --git a/altair/vegalite/v5/theme.py b/altair/vegalite/v5/theme.py
index a3c62165..097e7b30 100644
--- a/altair/vegalite/v5/theme.py
+++ b/altair/vegalite/v5/theme.py
@@ -53,6 +53,15 @@ VEGA_THEMES = [
 ]
 
 
+def _load_themes():
+    import json
+    from pathlib import Path
+
+    from altair.vegalite.v5 import schema
+
+    return json.loads((Path(*schema.__path__) / "vega-themes.json").read_text())
+
+
 class VegaTheme:
     """Implementation of a builtin vega theme."""
 

I'd prefer leaving it out for now, since there is quite a lot of variation between the themes.
Navigating them in an ipython environment is fine, but statically this alone isn't much help IMO

@dangotbanned dangotbanned mentioned this pull request Aug 7, 2024
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Aug 7, 2024
jonmmease pushed a commit that referenced this pull request Aug 7, 2024
* ci: Bump `vl-convert-python>=1.6.0`

Likely the source of #3523 (comment)

* ci: Bump `sphinx>=8.0.0`

Possibly resolves issue #3515 (comment)
@jonmmease
Copy link
Contributor

Sorry I wasn't very clear there. I wasn't suggesting we provide a function to do this. Just pointing our that an end-user could do this if they needed to for some reason.

"vox",
]
AltairThemes: TypeAlias = Literal["default", "opaque"]
VEGA_THEMES: list[str] = list(get_args(VegaThemes))
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @binste this combines both of our comments #3516 (comment)

@dangotbanned
Copy link
Member Author

Let's leave it open another day to see if there are any other opinions. Feel free to ping me in 24 hrs if we haven't heard anything and I'll merge it.

@jonmmease if you're available to merge

@dangotbanned dangotbanned requested a review from jonmmease August 8, 2024 19:13
Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

LGMT! Agree that with 24kb it's small enough to include directly. Merging as Jon has indicated that he's ok with it as well.

@binste binste merged commit 3e9faaa into vega:main Aug 8, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants