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

FIX: Remove sphinx-book-theme from extensions #2111

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

ghisvail
Copy link
Contributor

@ghisvail ghisvail commented Feb 5, 2024

Copy link

welcome bot commented Feb 5, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

ghisvail added a commit to ghisvail/medkit that referenced this pull request Feb 5, 2024
ghisvail added a commit to medkit-lib/medkit that referenced this pull request Feb 5, 2024
@agoose77 agoose77 merged commit 952245a into jupyter-book:master Feb 7, 2024
2 checks passed
Copy link

welcome bot commented Feb 7, 2024

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@agoose77
Copy link
Collaborator

agoose77 commented Feb 7, 2024

Great, thanks! This is a much-needed change.

@ghisvail ghisvail deleted the patch-1 branch February 7, 2024 12:17
@ghisvail
Copy link
Contributor Author

ghisvail commented Feb 7, 2024

Indeed, it would be worth publishing a patch release with this change.

People using readthedocs to host JB-based docs, need to run jupyter-book config before the regular build step using Sphinx. The broken config can be amended with a trivial sed call, but it's better to avoid it than documenting it, IMO.

@agoose77
Copy link
Collaborator

agoose77 commented Feb 8, 2024

This needs a bit of a rethink, actually.

There are a few separate problems interplaying, and this PR introduces a new one; sphinx-book-theme exposes directives that aren't available during LaTeX builds if we only inject the extension via the HTML theme.

I'm going to revisit the whole stack and see if we can support both usages.

@agoose77
Copy link
Collaborator

agoose77 commented Feb 8, 2024

OK, that took some digging.

The registration order is as follows:

  • sphinx::Application.setup_extension()
  • sphinx::Config.init_values()
  • is_theme ? noop : <config-inited>
  • sphinx::Builder.init()
  • is_theme? sphinx::Registry.load_extension() : noop
  • sphinx::Builder.create_template_bridge() # Need to have valid theme path
  • <builder-inited>

This tells use that:

  1. The setup() level configuration registration ensures that the theme search path (in create_template_bridge is correct. We can't use any events for this because the theme is set-up immediately before the path is required (before any events are dispatched).
  2. Extensions are registered first, meaning that any theme variables set here get clobbered. These variables are not set again later because the theme's extension has already been loaded. This requires that we use e.g. config-inited to set this value before the theme is required.

@agoose77
Copy link
Collaborator

agoose77 commented Feb 8, 2024

I think I have the solution in mind, now.

agoose77 added a commit that referenced this pull request May 28, 2024
This reverts commit 952245a, reversing
changes made to 30afdb7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants