-
Notifications
You must be signed in to change notification settings - Fork 35
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
Search: correctly remove Search.init on Sphinx >= 5.0 #100
Conversation
On sphinx-doc/sphinx#10028 the `Sphinx.init` call was changed to be called with the `_ready` function. We need to update our regex to catch that, otherwise `Sphinx.init` will be called twice. Closes readthedocs/readthedocs.org#8862.
sphinxlatest: Sphinx | ||
sphinxmaster: git+https://github.com/sphinx-doc/sphinx.git@master |
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'm not running sphinxmaster on circle since it could fail, but it's useful for testing future changes locally.
tox.ini
Outdated
@@ -1,7 +1,7 @@ | |||
[tox] | |||
envlist = | |||
py{27,36}-sphinx{15,16,17,18} | |||
py{36,37,38,39}-sphinx{21,22,23,24,30,31,32,33,34,latest} | |||
py{36,37,38,39,310}-sphinx{21,22,23,24,30,31,32,33,34,35,40,41,42,43,44,latest} |
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.
Looks like sphinx 3.5, 4.0 and 4.1 aren't compatible with python 3.10 (bc of changes on the typing module). Thoughts about testing 3.10 only for sphinx>=4.2?
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.
Sounds good to me.
^(\$\(document\).ready\(function\s*\(\)\s*{(?:\n|\r\n?) | ||
\s*Search.init\(\);(?:\n|\r\n?) | ||
\}\); | ||
\}\);) | ||
| | ||
# Sphinx >=5.0 calls Search.init this way. | ||
(_ready\(Search.init\);) | ||
''', |
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 it would be good to add links to the sphinx code we are expecting to search & replace as comments in this regex for easier tracking/understanding of it.
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.
@stsewd it seems you missed this comment.
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 updated the docstring ad92c76
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.
Oh, I see. I missed that :)
On sphinx-doc/sphinx#10028
the
Sphinx.init
call was changed to be called with the_ready
function.
We need to update our regex to catch that,
otherwise
Sphinx.init
will be called twice.Also, I have updated our test matrix to include new versions of sphinx/python.
And in the latest version,
language
can't beNone
, so I'm changing it to 'en'.Closes readthedocs/readthedocs.org#8862.