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

Pin version for Sphinx and extensions, configure hoverxref #4480

Merged
merged 8 commits into from Apr 15, 2020

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented Apr 10, 2020

It's green! ✔️

Closes #4475

docs/conf.py Outdated Show resolved Hide resolved
@@ -295,3 +295,5 @@
# ------------------------------------

hoverxref_auto_ref = True
hoverxref_project = "scrapy"
Copy link
Member Author

@elacuesta elacuesta Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this is necessary now, otherwise we get

hoverxref extension is not fully configured. Set hoverxref_project and hoverxref_version in your conf.py file.

Setting: https://sphinx-hoverxref.readthedocs.io/en/latest/configuration.html#confval-hoverxref_project
Failed build: https://travis-ci.org/github/scrapy/scrapy/jobs/673517936#L286

@elacuesta elacuesta closed this Apr 10, 2020
@elacuesta elacuesta reopened this Apr 10, 2020
@elacuesta elacuesta marked this pull request as draft Apr 10, 2020
@@ -295,3 +295,12 @@
# ------------------------------------

hoverxref_auto_ref = True
hoverxref_project = "scrapy"
hoverxref_version = release
hoverxref_role_types = {
Copy link
Member Author

@elacuesta elacuesta Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be necessary too, otherwise we get

Using default style for unknown typ. Define it in hoverxref_role_types. typ=ref style=tooltip

Setting:
https://sphinx-hoverxref.readthedocs.io/en/latest/configuration.html#confval-hoverxref_role_types
Failed build: https://travis-ci.org/github/scrapy/scrapy/jobs/673579524#L1209

@@ -1116,17 +1116,6 @@ multi-purpose thread pool used by various Scrapy components. Threaded
DNS Resolver, BlockingFeedStorage, S3FilesStore just to name a few. Increase
this value if you're experiencing problems with insufficient blocking IO.

.. setting:: REDIRECT_MAX_TIMES
Copy link
Member Author

@elacuesta elacuesta Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed to avoid errors about duplicate definitions, such as

/home/travis/build/scrapy/scrapy/docs/topics/settings.rst:1119:duplicate setting description of REDIRECT_MAX_TIMES, other instance in topics/downloader-middleware

https://travis-ci.org/github/scrapy/scrapy/jobs/673570891#L348

@elacuesta elacuesta marked this pull request as ready for review Apr 10, 2020
@codecov
Copy link

@codecov codecov bot commented Apr 10, 2020

Codecov Report

Merging #4480 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4480      +/-   ##
==========================================
- Coverage   84.79%   84.78%   -0.02%     
==========================================
  Files         164      164              
  Lines        9887     9887              
  Branches     1468     1468              
==========================================
- Hits         8384     8383       -1     
  Misses       1248     1248              
- Partials      255      256       +1     
Impacted Files Coverage Δ
scrapy/utils/trackref.py 82.85% <0.00%> (-2.86%) ⬇️

@elacuesta elacuesta added this to the v2.1 milestone Apr 10, 2020
elacuesta added a commit to elacuesta/scrapy that referenced this issue Apr 11, 2020
docs/requirements.txt Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
Co-Authored-By: Adrián Chaves <adrian@chaves.io>
@kmike kmike merged commit 885b0ad into scrapy:master Apr 15, 2020
2 checks passed
@kmike
Copy link
Member

@kmike kmike commented Apr 15, 2020

Thanks @elacuesta and @Gallaecio!

@elacuesta elacuesta deleted the pin-sphinx-versions branch Apr 15, 2020
@@ -295,3 +295,12 @@
# ------------------------------------

hoverxref_auto_ref = True
hoverxref_project = "scrapy"
hoverxref_version = release
Copy link

@humitos humitos Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! This breaks in production when rendered by Read the Docs.

You shouldn't override hoverxref_version and hoverxref_project since they are taken automatically from Read the Docs.

If you want to avoid your CI failing because of this, you can define the environment variables as Read the Docs does:

  • READTHEDOCS_PROJECT=scrapy
  • READTHEDOCS_VERSION=''

With the current configuration, all the versions built on Read the Docs will point to a different version on Read the Docs and this will conflict. For example, current master version in Read the Docs defines hoverxref_version='2.0.0' but that version does not exist on Read the Docs and the tooltip does not known where to get the content from.

Cheers,

Copy link
Member

@kmike kmike Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up @humitos!

Copy link
Member Author

@elacuesta elacuesta Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for looking into this @humitos! 🚀

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

Successfully merging this pull request may close these issues.

4 participants