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

Do not add HTML mixin if not building html #2076

Merged
merged 1 commit into from
Mar 7, 2025
Merged

Conversation

simonspa
Copy link
Contributor

When attempting to build the latexpdf target, pydata-sphinx-theme 0.16 currently fails on Sphinx 8.1.3 with:

Exception occurred:
  File "/home/simonspa/venv/lib64/python3.13/site-packages/pydata_sphinx_theme/translator.py", line 41, in visit_table
    self._table_row_indices.append(0)
    ^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'BootstrapHTML5Translator' object has no attribute '_table_row_indices'
The full traceback has been saved in /tmp/sphinx-err-7w3z72wv.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [Makefile:25: latexpdf] Error 1

The issue appears to be that the BootstrapHTML5Mixin is added despite the target not being html. This PR moves the check for app.builder.format to the top level of setup_translators.

(cc) @stephanlachnit

@trallard trallard requested a review from gabalafou December 16, 2024 11:23
@trallard trallard added kind: bug Something isn't working tag: HTML Issues related to HTML generation labels Dec 16, 2024
Copy link
Collaborator

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

This change makes sense to me, but just FYI, I am unable to reproduce the bug.

Using:

I ran make latexpdf but I did not get any errors.

Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/pydata_sphinx_theme
  translator.py 88
Project Total  

This report was generated by python-coverage-comment-action

@drammock
Copy link
Collaborator

I recommend waiting to merge until after 0.16.1 is out. This seems innocuous but I would hate to break a bunch of user sites and have to scramble to cut a _post release in a hurry.

@gabalafou gabalafou added the DO NOT MERGE There is something blocking merging this PR label Dec 16, 2024
@gabalafou
Copy link
Collaborator

@drammock, agreed

@trallard trallard added this to the 0.16.2 milestone Dec 17, 2024
@trallard trallard added DO NOT MERGE There is something blocking merging this PR and removed DO NOT MERGE There is something blocking merging this PR labels Dec 17, 2024
@simonspa
Copy link
Contributor Author

@gabalafou thanks to your minimal example I was able to find my mistake: I listed the pydata_sphinx_theme also under extensions, not only as html theme.

I anyway think the fix makes sense. :)

s-weigand added a commit to glotaran/pyglotaran-extras that referenced this pull request Mar 1, 2025
Today I found that our docs are broken due to a bug when building PDFs.

This PR changes the [pydata-sphinx-theme](https://github.com/pydata/pydata-sphinx-theme) from the published upstream project to a branch on my for including the following fixes:

- pydata/pydata-sphinx-theme#2097
- pydata/pydata-sphinx-theme#2076

I also went ahead and removed the workaround on our side since it isn't needed with the fix on my fork in place.

### Change summary

- [📚🩹 Use fixed pydata-sphinx-theme from fork](5f3bedf)
- [🚇🩹 Setup docs env same as on RTD](365899d)
@s-weigand
Copy link
Contributor

@simonspa Thanks for the hint in your comment I faced the same issue and it was also due to the usage error that I added pydata_sphinx_theme to extensions, which caused issues with all non HTML builders 🤦‍♀️

Might, be good to have a very prominent warning in the docs (no idea where the best place for that would be, so even sparse doc readers like me would see it) and/or warn/error if pydata_sphinx_theme is a key in app.extensions since adding a theme to the extensions is a very common pattern and an easy to make error, especially when switching themes.

@simonspa
Copy link
Contributor Author

simonspa commented Mar 7, 2025

Maybe it would make sense to merge these changes irrespectively? They seem innocuous enough but would allow using pydata_sphinx_theme even when listed as extension.

@drammock drammock removed the DO NOT MERGE There is something blocking merging this PR label Mar 7, 2025
@drammock drammock merged commit 9fc4799 into pydata:main Mar 7, 2025
25 checks passed
gabalafou pushed a commit to gabalafou/pydata-sphinx-theme that referenced this pull request Mar 10, 2025
When attempting to build the `latexpdf` target, pydata-sphinx-theme 0.16
currently fails on Sphinx 8.1.3 with:

```
Exception occurred:
  File "/home/simonspa/venv/lib64/python3.13/site-packages/pydata_sphinx_theme/translator.py", line 41, in visit_table
    self._table_row_indices.append(0)
    ^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'BootstrapHTML5Translator' object has no attribute '_table_row_indices'
The full traceback has been saved in /tmp/sphinx-err-7w3z72wv.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [Makefile:25: latexpdf] Error 1
```

The issue appears to be that the `BootstrapHTML5Mixin` is added despite
the target not being `html`. This PR moves the check for
`app.builder.format` to the top level of `setup_translators`.

(cc) @stephanlachnit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working tag: HTML Issues related to HTML generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants