Bugfix: Improve support for external theme packages by jdillard · Pull Request #134 · useblocks/sphinx-simplepdf

jdillard · 2026-03-10T19:42:41Z

Summary

The SimplePdfBuilder resolves the SCSS sources folder via a hardcoded relative path to the bundled simplepdf_theme. This means anyone using the simplepdf_theme config option to point at an external theme still gets SCSS compiled from the bundled theme rather than their own.

This PR adds a _resolve_scss_folder() method that dynamically imports the theme module specified by simplepdf_theme and calls its get_scss_sources_path() function to locate the SCSS sources. If the configured theme cannot be imported or doesn't define get_scss_sources_path(), the builder falls back to the bundled simplepdf_theme.

Changes

Replace hardcoded SCSS path with _resolve_scss_folder() method using import-based theme discovery
Add get_scss_sources_path() to the bundled simplepdf_theme so it follows the same contract as external themes
Added tests/test_external_theme.py with two integration tests:
- with_external_theme: stub theme package implements get_scss_sources_path(); assertions include generated _static/main.css containing a test-only SCSS marker.
- with_theme_missing_scss_hook: stub theme has no get_scss_sources_path(); asserts the builder warning and successful PDF build (bundled SCSS fallback).

docs preview build: https://sphinx-simplepdf--134.org.readthedocs.build/en/134/configuration.html#simplepdf-theme

Comparison with #91

PR #91 took a different approach: it moved SCSS compilation out of the builder and into the theme via the builder-inited event, with the goal of fully decoupling theme from builder. That approach had several issues raised in review, most notably:

It assumed all external themes would have a static/styles/sources/ directory with SCSS, breaking themes that only use app.add_css_file() to override styles
It required moving simplepdf_vars and simplepdf_theme_options registration into the theme, fragmenting configuration

This PR takes a simpler approach: keep SCSS compilation in the builder where it already lives, but make the SCSS source path resolution dynamic. Theme authors explicitly opt in by defining get_scss_sources_path() — there's no assumed directory structure and no restructuring of responsibilities. Themes that don't define it get a clear warning rather than silent breakage.

I think this is the better approach since theme's with the purpose of creating PDFs are more likely to be tightly coupled with the simplePDF extension.

Backward Compatibility

This change is fully backward-compatible. When simplepdf_theme is set to the default "simplepdf_theme", the bundled theme's get_scss_sources_path() is called and resolves to the same path as before.

Refactor SCSS folder resolution to use a method that imports the theme module and retrieves the SCSS sources path, with a fallback to the bundled theme.

…es_path() convention

patdhlk

Good design, clean approach, addresses the right problem. Needs one more pass on the error boundaries. Thanks for the PR 🙏

To summarize:

Catch Exception, not just ImportError — third-party code can fail in surprising ways
Validate the returned path exists (os.path.isdir) before handing it to sass
Drop the version bump — let the maintainer handle that
Consider normalizing with os.path.abspath() or at least document that the return must be absolute
Add a brief comment on why importlib.import_module(config_string) is acceptable here
Show evidence of a manual test with an actual external theme (<-- nice to have 👀 )

patdhlk · 2026-04-01T16:29:43Z

+        """
+        theme_name = self.app.config.simplepdf_theme or "simplepdf_theme"
+        try:
+            theme_module = importlib.import_module(theme_name)


You're importing an arbitrary Python module based on a config string. That's importlib.import_module(user_controlled_string). In practice the threat model is fine — anyone who can write conf.py already has code execution. But worth noting: simplepdf_theme is a config value that becomes a direct import. If this project ever grows a web-based config UI or accepts config from untrusted sources, this is an RCE. For now it's fine, but a one-line comment like # theme_name comes from conf.py, which is already arbitrary Python would tell the next person why this is OK.

comment added in 6f8658d

patdhlk · 2026-04-01T16:33:32Z

+        try:
+            theme_module = importlib.import_module(theme_name)
+            if hasattr(theme_module, "get_scss_sources_path"):
+                return theme_module.get_scss_sources_path()


You import the module, call one function, and trust whatever path it returns. What if get_scss_sources_path() returns a path that doesn't exist? Or
returns /etc/shadow? You feed it straight into sass.compile(dirname=(scss_folder, css_folder)). libsass will just fail with an opaque error.

One os.path.isdir() check after the call would turn a confusing sass traceback into a clear error message:

scss_folder = theme_module.get_scss_sources_path() if not os.path.isdir(scss_folder): raise ExtensionError( f"Theme '{theme_name}' get_scss_sources_path() returned " f"non-existent directory: {scss_folder}" ) return scss_folder

Maybe that's the only validation we need. No need to overthink it.

Addressed in 773595e

patdhlk · 2026-04-01T16:34:55Z

+        except ImportError:
+            logger.warning(
+                f"Could not import theme '{theme_name}', "
+                "falling back to bundled simplepdf_theme",
+                type="simplepdf",
+                subtype="theme",
+            )


You catch ImportError but not Exception. If someone's theme __init__.py raises a ValueError or AttributeError during import, you get an unhandled crash instead of a fallback. Either catch Exception here (with a more specific log message) or document that theme modules must import cleanly.

I'd catch Exception. Theme code is third-party; be defensive at the boundary.

Fixed in 2783135

patdhlk · 2026-04-01T16:35:58Z

 [project]
 name = "sphinx-simplepdf"
-version = "1.7.0"
+version = "1.8.0"


I'd leave it out of the PR and let the maintainer handle it. One less thing to merge-conflict on.

Fixed in 7ffa7cb

patdhlk · 2026-04-01T16:37:23Z

The changelog entry is fine. No tests, but there's no test harness in this project yet (PR #121). What I would want to see is: did you actually test this with a real external theme? The PR description says this fixes the issue from PR #91 differently, but there's no evidence of a manual test. A screenshot or build log showing an external theme's SCSS being compiled would go a long way.

I am using this change on an internal fork, with an internal theme and it's been working great. I did merge main, since #121 was merged, and added some tests in 3057a91, as well as updated the PR description to mention the new tests.

patdhlk · 2026-04-01T16:37:26Z

The docs update is good. Clear contract: implement get_scss_sources_path(), return an absolute path. The example is minimal and correct. The suppress_warnings note is a nice touch.

One thing missing from the docs: what happens if get_scss_sources_path() returns a relative path? sass.compile might resolve it relative to cwd, which is sometimes unpredictable in Sphinx builds. The docstring says "absolute path" but you don't enforce it. Either os.path.abspath() the result or document that it must be absolute and let it fail loudly 🧨

Addressed in 773595e: the builder now runs os.path.abspath on the return value and requires the path to be an existing directory before sass.compile

patdhlk · 2026-04-01T16:39:30Z

 Changelog
 =========

+Release 1.8


I'd leave it out of the PR and let the maintainer handle it. Mark/add it as unreleased

Fixed in 7ffa7cb

patdhlk · 2026-04-01T16:40:45Z

+                type="simplepdf",
+                subtype="theme",
+            )
+        from sphinx_simplepdf.themes.simplepdf_theme import get_scss_sources_path


This fallback import is at module scope inside a method. It works, but it's a little surprising. You already have importlib — why not just importlib.import_module("sphinx_simplepdf.themes.simplepdf_theme") for consistency?

Or just compute the path inline since you know exactly where it is. The late import buys you nothing here; this package is always installed.

Fixed in 670ddee

jdillard · 2026-04-02T00:23:46Z

Thanks for the review! I think addressed all the comments.

patdhlk

Thanks. Good to go now.

jdillard and others added 2 commits March 6, 2026 18:46

Support external theme packages for SCSS compilation

7f55f21

Refactor SCSS folder resolution to use a method that imports the theme module and retrieves the SCSS sources path, with a fallback to the bundled theme.

Improve support for external theme packages by using a get_scss_sourc…

f42b437

…es_path() convention

jdillard mentioned this pull request Mar 10, 2026

New Feature: Add notoc theme option #125

Open

Add PR reference to changelog bugfix entry

4c8e244

jdillard changed the title ~~Improve support for external theme packages~~ Bugfix: Improve support for external theme packages Mar 18, 2026

patdhlk self-requested a review April 1, 2026 16:26

patdhlk requested changes Apr 1, 2026

View reviewed changes

jdillard added 7 commits April 1, 2026 11:38

chore: keep package version on 1.7.0

7ffa7cb

Merge branch 'main' into external-theme

c02d5f7

test: add integration coverage for external theme SCSS resolution

3057a91

chore: document why import_module(theme_name) is acceptable

6f8658d

fix: harden theme SCSS resolution against third-party failures

2783135

refactor: load bundled theme SCSS via importlib

670ddee

fix: validate theme SCSS directory before sass.compile

773595e

Fix ruff formatting

3df70ca

patdhlk self-requested a review April 4, 2026 08:10

patdhlk approved these changes Apr 4, 2026

View reviewed changes

patdhlk merged commit cda8315 into useblocks:main Apr 4, 2026
25 checks passed

jdillard deleted the external-theme branch April 7, 2026 19:11

Conversation

jdillard commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Comparison with #91

Backward Compatibility

Uh oh!

patdhlk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdillard commented Apr 2, 2026

Uh oh!

patdhlk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jdillard commented Mar 10, 2026 •

edited

Loading