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

Show version for Sphinx and this template in footer. #1269

Closed
wants to merge 1 commit into from

Conversation

marxin
Copy link

@marxin marxin commented Dec 28, 2021

Emits now:

Screenshot from 2021-12-28 15-08-29

@marxin marxin requested a review from a team as a code owner December 28, 2021 14:09
@marxin
Copy link
Author

marxin commented Jan 6, 2023

MIT8

Can you please explain this? Is the community interested in the change, or should I close the pull request?

@benjaoming
Copy link
Contributor

@marxin I've really been wanting this, too 🎉

We have some issue with legacy installation setups, though. So if we introduced it as is, it would unfortunately give the same errors as described here:

#1355

@agjohnson moving forwards, we should probably put something on the roadmap about making legacy setups unsupported and find a way to detect them and fail nicely? On of the steps needed is also unfortunately on readthedocs.org - readthedocs/readthedocs.org#9654

@marxin
Copy link
Author

marxin commented Jan 13, 2023

Please make a similar PR if you want the feature in the future. This PR conflicts, thus I'm closing it.

@marxin marxin closed this Jan 13, 2023
@benjaoming
Copy link
Contributor

@marxin I was going over #1355 and recalled that it only failed because an undefined context variable was used in a way that it cannot.

If you implement it in a way that anticipates that rtd_theme_version may be None then it can work. For instance {%- if rtd_theme_version -%}(v{{ rtd_theme_version }}){%- endif -%}

Btw that "MIT8" comment is bot spam that we're unfortunately seeing a lot of on GitHub nowadays.

@marxin
Copy link
Author

marxin commented Jan 13, 2023

If you implement it in a way that anticipates that rtd_theme_version may be None then it can work. For instance {%- if rtd_theme_version -%}(v{{ rtd_theme_version }}){%- endif -%}

The question is: How can it be None if I set it in:
app.connect("html-page-context", update_context)
?

@benjaoming
Copy link
Contributor

That's the longer story that is uncovered in #1355 and readthedocs/readthedocs.org#9654

Basically, you cannot rely on app.connect to be executed in two cases:

  • Legacy configurations that didn't add the theme the correct way
  • On RTD sphinx_rtd_theme has to be added to extensions in conf.py which is both unnecessary and unconventional (due to an injection that RTD does, which is probably also related to legacy)

We are facing the same issue with the automatic enabling of sphinxcontrib-jquery which adds some priority to fixing the issue.

@marxin
Copy link
Author

marxin commented Jan 13, 2023

Sure, I've just made a new PR in #1410.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants