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: use config-inited event to register config #814

Merged
merged 13 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@ additional-compiled-static-assets = [
testpaths = [
"tests"
]
filterwarnings = [
"error",
'ignore:Jupyter is migrating its paths to use standard platformdirs:DeprecationWarning',
'ignore:The frontend\.OptionParser class will be replaced:DeprecationWarning',
'ignore:The frontend\.Option class will be removed:DeprecationWarning',
'ignore:nodes\.Node\.traverse\(\) is obsoleted by Node\.findall\(\):PendingDeprecationWarning',
# jupyter-client throws this
'ignore:datetime\.datetime\.utcfromtimestamp\(\) is deprecated:DeprecationWarning',
'ignore:datetime\.datetime\.utcnow\(\) is deprecated:DeprecationWarning',
# Sphinx triggers this
'''ignore:'imghdr' is deprecated and slated for removal in Python 3\.13:DeprecationWarning''',
]

[project]
name = "sphinx-book-theme"
Expand Down
42 changes: 12 additions & 30 deletions src/sphinx_book_theme/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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


Comment on lines -182 to -200
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just cleanup!

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


Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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:

  1. SBT is only used as a theme, which means config-inited has already triggered by the time we register the listener
  2. SBT is both a theme and extension, in which case the app.config in setup() is nearly immediately clobbered.

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)

Expand All @@ -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,
Expand Down
20 changes: 20 additions & 0 deletions src/sphinx_book_theme/directives.py
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")

Check warning on line 15 in src/sphinx_book_theme/directives.py

View check run for this annotation

Codecov / codecov/patch

src/sphinx_book_theme/directives.py#L12-L15

Added lines #L12 - L15 were not covered by tests

# Remove the "title" node if it is empty
if not self.arguments:
nodes[0].children.pop(0)
return nodes

Check warning on line 20 in src/sphinx_book_theme/directives.py

View check run for this annotation

Codecov / codecov/patch

src/sphinx_book_theme/directives.py#L18-L20

Added lines #L18 - L20 were not covered by tests
20 changes: 9 additions & 11 deletions tests/test_build.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import os
from pathlib import Path
from shutil import copytree, rmtree
from subprocess import check_call
from importlib.metadata import version
from packaging.version import parse

Expand All @@ -28,9 +27,10 @@ def __init__(self, app: SphinxTestApp, src: Path):
f".sphinx{sphinx.version_info[0]}" # software version tracking for fixtures
)

def build(self, assert_pass=True):
def build(self, assert_pass=True, assert_no_warnings=True):
self.app.build()
assert self.warnings == "", self.status
if assert_no_warnings:
assert self.warnings == "", self.status
return self

@property
Expand Down Expand Up @@ -62,14 +62,12 @@ def _func(src_folder, **kwargs):
yield _func


def test_parallel_build():
# We cannot use the sphinx_build_factory because SpinxTestApp does
# not have a way to pass parallel=2 to the Sphinx constructor
# https://github.com/sphinx-doc/sphinx/blob/d8c006f1c0e612d0dc595ae463b8e4c3ebee5ca4/sphinx/testing/util.py#L101
check_call(
f"sphinx-build -j 2 -W -b html {path_tests}/sites/parallel-build build",
shell=True,
)
def test_parallel_build(sphinx_build_factory):
sphinx_build = sphinx_build_factory("parallel-build", parallel=2) # type: SphinxBuild
sphinx_build.build(
assert_pass=True, assert_no_warnings=False
) # TODO: filter these warnings
assert (sphinx_build.outdir / "index.html").exists(), sphinx_build.outdir.glob("*")


def test_build_book(sphinx_build_factory, file_regression):
Expand Down
2 changes: 2 additions & 0 deletions tests/test_build/build__pagetoc--page-onetitlenoheadings.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<div class="bd-sidebar-secondary bd-toc">
</div>
Loading