search

readthedocs / sphinx_rtd_theme

Sphinx theme from Read the Docs
https://sphinx-rtd-theme.readthedocs.io/
MIT License
4.81k stars 1.74k forks source link

Do not use theme as extension, this should be made redundant #1479

Closed benjaoming closed 3 months ago

benjaoming commented 1 year ago

This seeks to demonstrate that our setup(app) block is called correctly and all subsequent functionality is called as intended. Including the call to jquery_add_js_files that was broken because themes cannot activate extensions.

https://github.com/readthedocs/sphinx_rtd_theme/blob/master/sphinx_rtd_theme/__init__.py#L41-L85

humitos commented 1 year ago

If this is true, we should update our installing instructions as well https://sphinx-rtd-theme.readthedocs.io/en/stable/installing.html

humitos commented 3 months ago

I don't 100% understand the Sphinx internals, but it seems we cannot remove the installation as an extension: https://github.com/readthedocs/sphinx_rtd_theme/issues/1434#issuecomment-2289443380

benjaoming commented 3 months ago

Hi @humitos

I recently had to deal with yet another instance of this bug, it keeps coming up :) It's because of defining html_theme_path in conf.py which should not be needed, but has been part of many many instructions and circulating copy-pasta.

In the case in question, it's defined here:

https://github.com/LGouellec/streamiz/blob/d1a0b73b2ebc5a82c8d00babf16b51f2a7423841/docs/conf.py#L61-L64

I tried to summarize the issue here: https://github.com/readthedocs/sphinx_rtd_theme/issues/1434#issuecomment-1472671651

Another thing is that maybe the need for html_theme_path needs to be understood better... but I think that the entire code block above should be possible to remove.

humitos commented 3 months ago

Hey 👋🏼 .

It's because of defining html_theme_path in conf.py which should not be needed, but has been part of many many instructions and circulating copy-pasta.

Yeah, it seems we removed this from our installation page a while ago now.

We still have the function get_html_theme_path() declared in our code. So, it would be probably a good idea to remove it from there and make those projects defining html_theme_path to hard fail; so they can upgrade.

benjaoming commented 3 months ago

@humitos that sounds like a good way forward because the missing JQuery causes confusion and can waste a lot of time in debugging and for the users that are impacted (negatively impacting for instance search). If possible, a potential compromise could be to have a DeprecationWarning lodged in an intermittent update, then remove it completely (or rename it, in case there's something unanticipated that really needs it).

humitos commented 3 months ago

If possible, a potential compromise could be to have a DeprecationWarning lodged in an intermittent update

I added a commit cc5067a (#1576) to our 3.0.0rc1 release 👍🏼 . Let me know if that works.