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

[drammock]

closes #1263
closes executablebooks/sphinx-book-theme#690

[drammock]

on second thought, there's a deeper problem here: @HealthyPear says they have accessible-pygments installed, and yet it's setting fallback Tango or Monokai. So below, line 958 ought to have picked up that the theme default (specified in our theme.conf) is a11y-high-contrast-{light,dark}:

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

Lines 955 to 958 in 956edcf

	# 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}")

...but it didn't, which means app.builder did not have a globalcontext attribute. To really fix this we need to figure out some way to reliably get the values from theme.conf other than through app.builder.globalcontext

[HealthyPear]

accessible-pygments v0.0.3 if it can be of any help

[drammock]

@HealthyPear could you test this branch to see if it now sets a11y-high-contrast-{light|dark} as your highlighting theme, and also doesn't generate the warning?

[drammock]

argh nevermind, what I have now works for rebuilds but not for first builds.

[drammock]

ok locally this seems to work for both builds and rebuilds. @HealthyPear can you test it on your site please?

[choldgraf]

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

[drammock]

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?

[choldgraf]

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

[drammock]

after further thought I don't think it's good to add app.builder.theme.get_options() to the helper func. There are basically two use cases:

1. we want the options dict so we can modify it. I don't think we ever want to modify during a build the options that were part of theme.conf
2. we want the options dict so we can query it.

I considered adding a separate helper func for the query case (to prevent accidental modification) but it seemed like overkill because in the end we only access the theme options 3 times in our __init__, and they're all different:

• modify theme options in update_config()
• query pygment_{light|dark}_style
• query logo and use {} as fallback

the second one we fall back to theme.conf values, but in the third one we actually don't want to check/fall back to theme.conf values (which will be None), we want {} instead.

Ready for merge when CIs are done

[12rambau]

thanks for putting all this together !

[HealthyPear]

ok locally this seems to work for both builds and rebuilds. @HealthyPear can you test it on your site please?

sorry for the delay!

I copy/pasted the edits of this merge request into the corresponding file within my conda environment and the problem seems to have disappeared - you have my delayed positive feedback :)

[HealthyPear]

Do you plan to make a bugfix release?

I am sure someone set their Sphinx build to error on warning and this bug would be a stopper for them (obviously totally unrelated to my use case)

[drammock]

Do you plan to make a bugfix release?

hopefully tomorrow. @12rambau if you have bandwidth to do this today, I'd say go for it.

[12rambau]

0.13.2 including your fix is out

[drammock]

@12rambau 🙏🏻🙏🏻🙏🏻