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

Add support for Sphinx 1.4 #1893

Merged
merged 1 commit into from Mar 30, 2016
Merged

Add support for Sphinx 1.4 #1893

merged 1 commit into from Mar 30, 2016

Conversation

@redapple
Copy link
Contributor

@redapple redapple commented Mar 30, 2016

See http://www.sphinx-doc.org/en/stable/changes.html#release-1-4-released-mar-28-2016

  • sphinx_rtd_theme has become optional, needs to be added to reqs
  • sphinx-doc/sphinx#2320 changes node entries tuples to 5 values instead of 4
  • sh syntax highlighting added very locally in selectors.rst because of this warning/error with Sphinx 1.4:
Warning, treated as error:
/home/paul/src/scrapy/docs/topics/selectors.rst:743:
WARNING: Could not lex literal_block as "python". Highlighting skipped.
See http://www.sphinx-doc.org/en/stable/changes.html#release-1-4-released-mar-28-2016

sphinx_rtd_theme has become optional, needs to be added to reqs

sphinx-doc/sphinx#2320 changes node entries tuples
to 5 values instead of 4

`sh` syntax highlighting added very locally in selectors.rst
because of this warning/error with Sphinx 1.4:

```
Warning, treated as error:
/home/paul/src/scrapy/docs/topics/selectors.rst:743:
WARNING: Could not lex literal_block as "python". Highlighting skipped.
```
@codecov-io
Copy link

@codecov-io codecov-io commented Mar 30, 2016

Current coverage is 83.18%

Merging #1893 into master will not affect coverage as of 752321e

Powered by Codecov. Updated on successful CI builds.

@kmike
Copy link
Member

@kmike kmike commented Mar 30, 2016

Hi Paul,

This looks good, but I think we should specify Sphinx 1.4+ dependency somewhere, it looks like some of the changes won't work with earlier Sphinx versions.

@redapple
Copy link
Contributor Author

@redapple redapple commented Mar 30, 2016

@kmike , which change you mean?
I tried with Sphinx 1.3.6 (forcing <1.4 in tox.ini) and it worked for me.
i'm ok with Sphinx>=1.3.6

@kmike
Copy link
Member

@kmike kmike commented Mar 30, 2016

ah, yeah, sorry, I thought "changes node entries tuples to 5 values instead of 4" is backwards incompatible, but your code handles both 4 and 5.

@kmike kmike merged commit a38a99e into scrapy:master Mar 30, 2016
2 checks passed
2 checks passed
codecov/patch coverage not affected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nyov
Copy link
Contributor

@nyov nyov commented Mar 30, 2016

Thanks, @redapple !

@harshasrinivas
Copy link
Contributor

@harshasrinivas harshasrinivas commented Mar 19, 2017

@redapple @kmike , since sphinx_rtd_theme has to be installed manually for building the docs, right now, it has only been added to the tox.ini file. So wouldn't it be better to add it to the requirements.txt as well? Or it could be added to docs/README.rst, something like

pip install 'Sphinx >= 1.3' sphinx_rtd_theme

What do you think?

@kmike
Copy link
Member

@kmike kmike commented Mar 20, 2017

@harshasrinivas I think we shouldn't add sphinx to requirements.txt - it is a file with packages needed to run Scrapy; users generally don't build Scrapy docs, so sphinx is not needed for them.

+1 to adding it to docs/README.rst; current instructions don't look complete, a good catch.

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

Successfully merging this pull request may close these issues.

None yet

5 participants