Closed
Conversation
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.
Closed
a07dba2 to
f784688
Compare
…es_path() convention
f784688 to
f42b437
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
SimplePdfBuilderresolves the SCSS sources folder via a hardcoded relative path to the bundledsimplepdf_theme. This means anyone using thesimplepdf_themeconfig 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 bysimplepdf_themeand calls itsget_scss_sources_path()function to locate the SCSS sources. If the configured theme cannot be imported or doesn't defineget_scss_sources_path(), the builder falls back to the bundledsimplepdf_themeand emits a Sphinx typed warning (simplepdf.theme) that users can suppress viasuppress_warnings.Changes
_resolve_scss_folder()method using import-based theme discoveryget_scss_sources_path()to the bundledsimplepdf_themeso it follows the same contract as external themestype="simplepdf",subtype="theme") instead of silently falling back when an external theme can't be loadedComparison with useblocks#91
PR #91 took a different approach: it moved SCSS compilation out of the builder and into the theme via the
builder-initedevent, with the goal of fully decoupling theme from builder. That approach had several issues raised in review, most notably:static/styles/sources/directory with SCSS, breaking themes that only useapp.add_css_file()to override stylessimplepdf_varsandsimplepdf_theme_optionsregistration into the theme, fragmenting configurationThis 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_themeis set to the default"simplepdf_theme", the bundled theme'sget_scss_sources_path()is called and resolves to the same path as before.