Skip to content

Fix Sphinx 9 compatibility for CSS and JS asset rendering #25

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.

Sign up for GitHub

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

Merged
mdickinson merged 7 commits into from
Feb 16, 2026
Merged

Fix Sphinx 9 compatibility for CSS and JS asset rendering #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d86b503
Fix Sphinx 9 compatibility for CSS and JS asset rendering
dpinte Feb 10, 2026
a8f2895
Address PR review: fix JS asset check and document Sphinx >= 4.5 requ…
dpinte Feb 13, 2026
88c6f4f
Restore fallback path, but without the 'is not none'
mdickinson Feb 16, 2026
0d83965
Revert Sphinx requirement
mdickinson Feb 16, 2026
f0e5c7f
Revert unnecessary change in README.rst
mdickinson Feb 16, 2026
3f2ea65
Bump version number in setup.py
mdickinson Feb 16, 2026
acda488
Add release date
mdickinson Feb 16, 2026
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
9 changes: 9 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
Enthought Sphinx Theme changelog
================================

Release 0.7.4
-------------

Release date: 2026-02-16

Changes

* Fix Sphinx 9 compatibility for CSS and JS asset rendering. (#25)

Comment on lines +4 to +12
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

The new 0.7.4 changelog section is missing a “Release date:” line, while all existing releases in this file include one. To keep the changelog consistent (and avoid ambiguity around whether 0.7.4 is actually released), add a release date or explicitly mark the section as unreleased.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@mdickinson mdickinson Feb 16, 2026

Choose a reason for hiding this comment

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

Addressed in acda488

Release 0.7.3
-------------

Expand Down
16 changes: 12 additions & 4 deletions enthought_sphinx_theme/enthought/layout.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,12 @@
HAS_SOURCE: {{ has_source|lower }}
};
</script>
{%- for scriptfile in script_files %}
<script type="text/javascript" src="{{ pathto(scriptfile, 1) }}"></script>
{%- for js in script_files %}
{%- if js|attr("filename") %}
{{ js_tag(js) }}
{%- else %}
<script type="text/javascript" src="{{ pathto(js, 1) }}"></script>
{%- endif %}
{%- endfor %}
Comment on lines +105 to 111
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

js|attr("filename") is not none will also be true for plain string entries because attr("filename") typically yields an Undefined value (which is not none). That would route strings into js_tag(js), likely failing and reintroducing the incompatibility. Prefer a type-based check (e.g., test whether js is a string and use pathto only in that case) or explicitly check that the attribute is defined (e.g., js|attr("filename") is defined and not none). Apply the same approach consistently to CSS/JS handling to avoid subtle differences in behavior.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@mdickinson mdickinson Feb 11, 2026

Choose a reason for hiding this comment

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

Hmm. This does seem like a valid point. IIUC, the code is attempting to be backwards compatible with older versions of Sphinx - for those older versions, js is a plain string and so js|attr("filename") will be Undefined (at least according to the docs). Which is not none. If this analysis is correct, we never actually take the fallback path.

@dpinte What does Claude say about this?

Copy link
Copy Markdown
Member Author

@dpinte dpinte Feb 13, 2026

Choose a reason for hiding this comment

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

Claude said you and copilot are right. It also inspected the standard upstream themes and verified how the checks were done. It seems they were not checking anything and thus just called js_tag(js). We'll remove those.

Copy link
Copy Markdown
Member

@mdickinson mdickinson Feb 16, 2026

Choose a reason for hiding this comment

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

Thanks, @dpinte.

The fallback code was a good idea, though - it saves us from having to worry about pinning a minimum Sphinx version. (Of course, the upstream code doesn't need to worry about this - it's shipped with Sphinx, so it can assume the latest Sphinx bells and whistles.)

I'll update the PR to do the right thing (which is to just remove the is not none check).

<script type="text/javascript" src="{{ pathto('_static/js/copybutton.js', 1) }}"></script>
<script type="text/javascript" src="{{ pathto('_static/js/wrap_on_dot.js', 1) }}"></script>
Expand All @@ -114,8 +118,12 @@
<link rel="stylesheet" type="text/css" href="{{ pathto('_static/css/spc-extend.css', 1) }}">
<link rel="stylesheet" href="{{ pathto('_static/' + styles[0], 1) }}" type="text/css" >
<link rel="stylesheet" href="{{ pathto('_static/pygments.css', 1) }}" type="text/css" >
{%- for cssfile in css_files %}
<link rel="stylesheet" href="{{ pathto(cssfile, 1) }}" type="text/css" >
{%- for css in css_files %}
{%- if css|attr("filename") %}
{{ css_tag(css) }}
{%- else %}
<link rel="stylesheet" href="{{ pathto(css, 1) }}" type="text/css" >
{%- endif %}
{%- endfor %}
{%- endmacro %}

Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

setup(
name='enthought_sphinx_theme',
version='0.7.3',
version='0.7.4',
author='Enthought, Inc.',
author_email='info@enthought.com',
description='Sphinx theme for Enthought products',
Expand Down