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

don't warn on fallback if no highlight theme was requested #1264

Merged
merged 6 commits into from
Mar 28, 2023
Merged

don't warn on fallback if no highlight theme was requested #1264

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
595c120
don't warn on fallback if no highlight theme requested
drammock Mar 23, 2023
8cf1969
fix it for good
drammock Mar 26, 2023
dc303d9
move event back to build-finished
drammock Mar 26, 2023
c6421dc
better name, better doc
drammock Mar 28, 2023
f8253f2
cleanup
drammock Mar 28, 2023
1c952b2
simplify/safer
drammock Mar 28, 2023
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
15 changes: 7 additions & 8 deletions src/pydata_sphinx_theme/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,17 +951,16 @@ def _overwrite_pygments_css(app, exception=None):
# see if user specified a light/dark pygments theme, if not, use the
# one we set in theme.conf
style_key = f"pygment_{light_or_dark}_style"

# globalcontext sometimes doesn't exist so this ensures we do not error
theme_name = _get_theme_options(app).get(style_key, None)
if theme_name is None and hasattr(app.builder, "globalcontext"):
theme_name = app.builder.globalcontext.get(f"theme_{style_key}")

if theme_name is None:
theme_name = app.builder.theme.get_options()[style_key]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that you might be able to use this helper function we use elsewhere:

pydata-sphinx-theme/src/pydata_sphinx_theme/__init__.py

Line 35 in 58caad4

def _get_theme_options(app):

If calling get_options() is the "right" way to do this, maybe we should update that function too. Gah there are way too many ways to do the same thing in Sphinx

Copy link
Collaborator Author

@drammock drammock Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am already using that helper function 2 lines above; this line is only called if the style key wasn't found by the helper. Most of this you already know @choldgraf, but for completeness / future selves:

  • app.builder.get_theme_config() returns

    • app.builder.config.html_theme (just the theme name)
    • app.builder.config.html_theme_options (dict of theme options). initialized here as an empty dict, it's not clear to me when it's populated, and with what it's populated directly from user's conf.py during app.config.read().

  • app.builder.theme_options is a copy of app.builder.config.html_theme_options.

    • The copy is made during builder.init_templates()
    • Apparently there are cases where that copy never happens (e.g., linkcheck?) so we have that helper function that checks app.builder.config.html_theme_options too.

  • Calling app.builder.theme.get_options() will get just the values in theme.conf without any user overrides.

    • Normally it's called by sphinx as .get_options(app.builder.theme_options), which makes me strongly suspect that app.builder.theme_options includes only the user-provided html_theme_options from their conf.py (and not the theme's defaults). EDIT see strikethrough edit above, it's clear now that html_theme_options does not include theme default values from theme.conf
    • Here's where it's called during sphinx builds: during builder.prepare_writing(), right after globalcontext is created.
    • I suspect our problem has been that builder.prepare_writing() is sometimes never called (e.g. if no files have changed?). That would explain why we had to work around missing globalcontext and also why the theme's defaults weren't available/accessible.

In this case, I think it would be possible / sensible to update the helper function to also check the theme.conf values. Might be a good use case for ChainMap?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sphinx never ceases to amaze me lol. Thanks for putting all of this together!

# make sure we can load the style
if theme_name not in pygments_styles:
logger.warning(
f"Color theme {theme_name} not found by pygments, falling back to {fallback}."
)
# only warn if user asked for a highlight theme that we can't find
if theme_name is not None:
logger.warning(
f"Color theme {theme_name} not found by pygments, falling back to {fallback}."
)
theme_name = fallback
# assign to the appropriate variable
if light_or_dark == "light":
Expand Down