Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Doc: Fix search for sphinx >=1.8#12216
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Doc: Fix search for sphinx >=1.8#12216
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 in2603ebc
| fromPILimportPILLOW_VERSION | |
| fromdistutils.versionimportLooseVersion | |
| ifLooseVersion(PILLOW_VERSION)>="3.4": |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 removesphinx!=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 intoreadthedocs/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.xBackport PR#12216 on branch v3.0.x (Doc: Fix search for sphinx >=1.8)
…216-on-v3.0.0-docBackport 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. ...
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Sphinx 1.8 broke the matplolib search as seen in#12183. The issue issphinx-doc/sphinx#5460 but it will not be fixed from the sphinx side. Instead they propose a solution as seen inreadthedocs/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 thelink 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