Doc: Fix search for sphinx >=1.8 #12216
Conversation
| {%- for scriptfile in script_files %} | ||
| <script type="text/javascript" src="{{ pathto(scriptfile, 1) }}"></script> | ||
| {%- endfor %} | ||
| {% if sphinx_version >= "1.8.0" %} |
There was a problem hiding this comment.
I think comparing versions as strings is a bad idea. For example, if Sphinx 1.10.0 is released, that version would be considered lower than 1.8.0 with such comparison.
There was a problem hiding this comment.
I took over the proposed fix (readthedocs/sphinx_rtd_theme#672).
I don't know if Sphinx has a hard policy on using one digit versioning, but at least it looks like 1.8.x is planned to be followed by 2.0.0.
In the matplotlib python code we use constructs like
matplotlib/lib/matplotlib/backend_bases.py
Lines 55 to 57 in 2603ebc
But I'm not familiar enough with the macro language that is used in the html document to judge on whether this could be used there as well.
There was a problem hiding this comment.
LooseVersion() will actually parse the version string to do a proper comparison, so it's not the same as here.
Doesn't mean we can't move forward on this as a practical solution.
There was a problem hiding this comment.
Even though string comparison for a version is not great, I think this is good enough given that sphinx 1.8 is likely followed by 2.0. The worst that can happen anyway is that this will break again.
So, I'm 👍 with moving this forward. Btw. can we then remove sphinx!=1.8.0 again? Would be less clutter in the doc_requirements.
|
I agree that doing better version parsing would be ideal, but am happy with this as a practical solution. |
|
This is the same solution that also went into readthedocs/sphinx_rtd_theme#672. It was confirmed that there is no plan to release something like "1.10", so this should be safe as it is. Concerning "remove sphinx!=1.8.0", I guess that makes sense, but there is still the question what to do about the incorrect rendering of math symbols. |
|
I didn’t find the math issue. So your’re saying that this needs a fix as well - or pinning to sphinx<1.8.0 for the time being? Anyway, I’m merging this because it’s a separate topic. |
…216-on-v3.0.x Backport PR #12216 on branch v3.0.x (Doc: Fix search for sphinx >=1.8)
…216-on-v3.0.0-doc Backport PR #12216 on branch v3.0.0-doc (Doc: Fix search for sphinx >=1.8)
|
Math issue is now tracked in #12532. |
* master: (51 commits) Disable sticky edge accumulation if no autoscaling. Avoid quadratic behavior when accumulating stickies. Rectified plot error (matplotlib#12501) endless looping GIFs with PillowWriter (matplotlib#11789) TST: add test for re-normalizing image FIX: colorbar re-check norm before draw for autolabels Fix search for sphinx >=1.8 (matplotlib#12216) Fix some flake8 issues Don't handle impossible values for `align` in hist() DOC: add section about test / doc dependencies DOC: clarify time windows for support TST: test colorbar tick labels correct FIX: make minor ticks formatted with science formatter as well DOC: clarify what 'minor version' means Adjust the widths of the messages during the build. Simplify radar_chart example. Fix duplicate condition in pathpatch3d example (matplotlib#12495) Fix typo in documentation FIX: make unused spines invisible Kill FontManager.update_fonts. ...
PR Summary
Sphinx 1.8 broke the matplolib search as seen in #12183. The issue is sphinx-doc/sphinx#5460 but it will not be fixed from the sphinx side. Instead they propose a solution as seen in readthedocs/sphinx_rtd_theme#672.
This makes it necessary to change the
layout.htmlmanually to include some new javascript file that defines some variables that are needed by the javascript that does the searching.This PR is changing the
layout.htmlin the same manner, such that the seach is functional again.Here is the link to the operational search.
At the moment sphinx 1.8.0 is excluded in the requirements, but since 1.8.1 is out already, it'll be used by automatically by test environments etc. It remains to be discussed whether sphinx should be pinned to 1.7.9 until the remaining issues (related to the change of
:math:) are fixed.PR Checklist