-
Notifications
You must be signed in to change notification settings - Fork 200
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: use config-inited
event to register config
#814
Changes from all commits
4e875d6
fce9b33
b6c6d4c
23fc8f9
5339a48
d1c49f0
85b4105
3099805
f0482f7
18b1041
c7129c1
6af8015
41aebcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,13 +4,13 @@ | |
from pathlib import Path | ||
from functools import lru_cache | ||
|
||
from docutils.parsers.rst.directives.body import Sidebar | ||
from docutils import nodes as docutil_nodes | ||
from sphinx.application import Sphinx | ||
from sphinx.locale import get_translation | ||
from sphinx.util import logging | ||
from pydata_sphinx_theme.utils import get_theme_options_dict | ||
|
||
from .directives import Margin | ||
from .nodes import SideNoteNode | ||
from .header_buttons import ( | ||
prep_header_buttons, | ||
|
@@ -179,32 +179,9 @@ def check_deprecation_keys(app): | |
) | ||
|
||
|
||
class Margin(Sidebar): | ||
"""Goes in the margin to the right of the page.""" | ||
|
||
optional_arguments = 1 | ||
required_arguments = 0 | ||
|
||
def run(self): | ||
"""Run the directive.""" | ||
if not self.arguments: | ||
self.arguments = [""] | ||
nodes = super().run() | ||
nodes[0].attributes["classes"].append("margin") | ||
|
||
# Remove the "title" node if it is empty | ||
if not self.arguments: | ||
nodes[0].children.pop(0) | ||
return nodes | ||
|
||
|
||
def update_general_config(app): | ||
theme_dir = get_html_theme_path() | ||
# Update templates for sidebar. Needed for jupyter-book builds as jb | ||
# uses an instance of Sphinx class from sphinx.application to build the app. | ||
# The __init__ function of which calls self.config.init_values() just | ||
# before emitting `config-inited` event. The init_values function overwrites | ||
# templates_path variable. | ||
|
||
app.config.templates_path.append(os.path.join(theme_dir, "components")) | ||
|
||
|
||
|
@@ -245,11 +222,20 @@ def setup(app: Sphinx): | |
app.connect("builder-inited", check_deprecation_keys) | ||
app.connect("builder-inited", update_sourcename) | ||
app.connect("builder-inited", update_context_with_repository_info) | ||
app.connect("builder-inited", update_general_config) | ||
app.connect("html-page-context", add_metadata_to_page) | ||
app.connect("html-page-context", hash_html_assets) | ||
app.connect("html-page-context", update_templates) | ||
|
||
# This extension has both theme-like and extension-like features. | ||
# Themes are initialised immediately before use, thus we cannot | ||
# rely on an event to set the config - the theme config must be | ||
# set in setup(app): | ||
update_general_config(app) | ||
# Meanwhile, extensions are initialised _first_, and any config | ||
# values set during setup() will be overwritten. We must therefore | ||
# register the `config-inited` event to set these config options | ||
app.connect("config-inited", update_general_config) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that for some folks if the config initialization was called twice, it would raise an error. If that's not the case then I think it's fine to just do this twice as long as nobody reports regressions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @choldgraf to be clear, I don't think this should ever set the config twice. AFAICT either:
I'm hoping that isn't a cause of bugs, and I've not seen any evidence as yet to suggest it might be 🤞 |
||
|
||
# Nodes | ||
SideNoteNode.add_node(app) | ||
|
||
|
@@ -266,10 +252,6 @@ def setup(app: Sphinx): | |
# Post-transforms | ||
app.add_post_transform(HandleFootnoteTransform) | ||
|
||
# Update templates for sidebar, for builds where config-inited is not called | ||
# (does not work in case of jupyter-book) | ||
app.config.templates_path.append(os.path.join(theme_dir, "components")) | ||
|
||
return { | ||
"parallel_read_safe": True, | ||
"parallel_write_safe": True, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
from docutils.parsers.rst.directives.body import Sidebar | ||
|
||
|
||
class Margin(Sidebar): | ||
"""Goes in the margin to the right of the page.""" | ||
|
||
optional_arguments = 1 | ||
required_arguments = 0 | ||
|
||
def run(self): | ||
"""Run the directive.""" | ||
if not self.arguments: | ||
self.arguments = [""] | ||
nodes = super().run() | ||
nodes[0].attributes["classes"].append("margin") | ||
|
||
# Remove the "title" node if it is empty | ||
if not self.arguments: | ||
nodes[0].children.pop(0) | ||
return nodes | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
<div class="bd-sidebar-secondary bd-toc"> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just cleanup!