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 rstcheck to CI #3624

Merged
merged 31 commits into from May 24, 2018

Conversation

Projects
None yet
4 participants
@stsewd
Member

stsewd commented Feb 16, 2018

This is an initial step to have more consistent docs. Closes #3622

Depends on #3498 (since some errors are solved there)

tox.ini Outdated
changedir = {toxinidir}/docs
commands =
rstcheck `find **/*.rst | xargs`

This comment has been minimized.

@stsewd

stsewd Feb 16, 2018

Member

I'm not sure if this is the right place, but looks logical to me, what I don't like is to have to install all the deps from lint.txt. Maybe adding a docs.txt?

This comment has been minimized.

@stsewd

stsewd Feb 16, 2018

Member

Also the --recursive option from rstcheck was added reciently myint/rstcheck@6102bec, so when that version gets released I will update the command :)

This comment has been minimized.

@humitos

humitos Feb 20, 2018

Member

It's already released, 3.2.

I also noted that you made a PR there! Hehe! Nice job! :)

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 16, 2018

After seen the results from travis I will add some rules to a .rstcheck.cfg file and see what problems we can solve.

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 16, 2018

Looks like currently the tool isn't very mature, we can't specify what errors to ignore (there are some very specific cases that are false positives). Anyway, I think this could be a good improvement to merge in the future :). I'm going to see if I can collaborate on rstcheck.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Feb 18, 2018

Big +1 on this, but generally adding linting requires a lot of fixing before merging.

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 18, 2018

@ericholscher yeah, probably we may want to do separate PRs for fixing some problems, and keep this branch up to date with master.

@humitos

This comment has been minimized.

Member

humitos commented Feb 20, 2018

Seems they are not that many:

api/bookmarks.rst:5: (INFO/1) Duplicate implicit target name: "".
api/builds.rst:5: (INFO/1) Duplicate implicit target name: "".
api/core.rst:5: (INFO/1) Duplicate implicit target name: "".
api/doc_builder.rst:5: (INFO/1) Duplicate implicit target name: "".
api/projects.rst:5: (INFO/1) Duplicate implicit target name: "".
api/vcs_support.rst:5: (INFO/1) Duplicate implicit target name: "".
business/index.rst:2: (INFO/1) Duplicate explicit target name: "readthedocs.com".
custom_installs/local_rtd_vm.rst:2: (INFO/1) Enumerated list start value not ordinal-1: "2" (ordinal 2)
dmca/index.rst:4: (INFO/1) Hyperlink target "our process" is not referenced.

I think we can fix them in this PR also, right?

@stsewd

This comment has been minimized.

Member

stsewd commented Feb 20, 2018

@humitos the first errors target name: "" are false positives, I'm already collaborating to rstcheck so we can used in a more configurable way :)

tox.ini Outdated
changedir = {toxinidir}/docs
commands =
bash -c 'rstcheck `find **/*.rst | xargs`'

This comment has been minimized.

@humitos

humitos Feb 20, 2018

Member

I think we should create a [testenv:doclint] with the rstcheck linting process instead of making it part of the docs building

tox.ini Outdated
commands =
sphinx-build -b html -d {envtmpdir}/doctrees . {envtmpdir}/html
[testenv:docs-lint]
deps = -r{toxinidir}/requirements/lint.txt

This comment has been minimized.

@humitos

humitos Feb 21, 2018

Member

Here. You just need rstcheck instead all the linting packages, I'd say

(or maybe creating another file docs-lint.txt?)

This comment has been minimized.

@stsewd

stsewd Feb 21, 2018

Member

We need also sphinx for more integration https://github.com/myint/rstcheck#sphinx

This comment has been minimized.

@humitos

humitos Feb 21, 2018

Member

Nice! So, I'd say that's better to have a separate reqs file.

@@ -0,0 +1,2 @@
-r pip.txt

This comment has been minimized.

@stsewd

stsewd Feb 21, 2018

Member

Not sure if we need all the packages from pip.txt, but just to follow similar convention, and we need Sphinx installed.

This comment has been minimized.

@humitos

humitos Feb 21, 2018

Member

No we don't need them. That's the idea of using a different file.

Add here just the packages that are needed. As far as I know, sphinx and rstcheck (also, do not pin them)

This comment has been minimized.

@humitos

humitos Feb 21, 2018

Member

No we don't need them. That's the idea of using a different file.

Add here just the packages that are needed. As far as I know, sphinx and rstcheck (also, do not pin them)

.travis.yml Outdated
@@ -7,6 +7,8 @@ matrix:
include:
- python: 2.7
env: TOXENV=docs
- python: 2.7

This comment has been minimized.

@humitos

humitos Feb 21, 2018

Member

Py3? :)

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 17, 2018

The 3.3 version of rstcheck was released! https://github.com/myint/rstcheck#id1

Soon I'll be updating this PR :)

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 17, 2018

Done, I made sure that all the links were shown correctly and that the deletes were not used in other parts of the docs.

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 17, 2018

Also, sorry for so many commits, you can squashed into one if you want

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 17, 2018

I missed one file, now the linter for docs should pass.

[rstcheck]
ignore_directives=automodule,http:get
ignore_roles=djangosetting,setting
ignore_messages=(Duplicate implicit target name: ".*")|(Hyperlink target ".*" is not referenced)

This comment has been minimized.

@stsewd

stsewd Mar 17, 2018

Member

Those errors are from some hyperlinks that are referenced globally. The duplicates are really false positives.

This comment has been minimized.

@humitos

humitos Mar 19, 2018

Member

By ignoring this now that are false positivies, we won't detect the ones that are true when we updated the docs :(

This comment has been minimized.

@stsewd

stsewd Mar 19, 2018

Member

Yeah, I going to try to solve others and see if I can get a better regex here

This comment has been minimized.

@stsewd

stsewd Mar 19, 2018

Member

There is a better way to expose global hiperlinks? Maybe something like this https://stackoverflow.com/a/19543591/5689214? And for the duplicates target name the most are from subheaders with the same name.

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Why not use the :ref: directive for linking across files?

http://www.sphinx-doc.org/es/stable/markup/inline.html#role-ref

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

rstcheck doesn't search for global refs and are marked as unused. But using ref is fine.

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

And also I change others place to use the doc directive https://docs.readthedocs.io/en/latest/docs.html#content.

@humitos

I think this is a really good improve to have automatically triggered in our PRs.

Regarding the ignore_messages, I'd say that it's good enough for now. Having this and ignore just 2 messages it too much better than ignoring everything as we were doing before this PR.

Optimization: is it possible to trigger this docs-lint env _only when there are changes in the docs/ directory?

@@ -4,32 +4,36 @@ DMCA Takedown Policy
These are the guidelines that Read the Docs follows when handling DMCA takedown
requests and takedown counter requests. If you are a copyright holder wishing to
submit a takedown request, or an author that has been notified of a takedown
request, please familiarize yourself with `our process <Takedown Process_>`_.
request, please familiarize yourself with `our process`__.

This comment has been minimized.

@humitos

humitos Mar 19, 2018

Member

Why we don't leave the extended name here? Isn't it clearer to understand where this will go?

This comment has been minimized.

@humitos

humitos Mar 19, 2018

Member

(the same for the other changes in this file)

This comment has been minimized.

@stsewd

stsewd Mar 19, 2018

Member

rstcheck didn't recognize those and marked it as errors. And I didn't find that syntax on the quickref http://docutils.sourceforge.net/docs/user/rst/quickref.html#internal-hyperlink-targets.

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member
`our process <Takedown Process_>`_

links to the "Takedown Process" title (L20 in dcma/index.rst)

If you remove the title from the link, like this

`our process`__

where that link will follow you? If you build the docs locally, you will find that link doesn't work.

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

The link is referenced just below https://github.com/rtfd/readthedocs.org/pull/3624/files#diff-1df69b2724a0d4a6195621dcef6dec3cR11

I see the link just fine on my browser

docsdmca

(see the link on the bottom)

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

My mouse is on our process but it doesn't appear on the screeenshot 😁

[rstcheck]
ignore_directives=automodule,http:get
ignore_roles=djangosetting,setting
ignore_messages=(Duplicate implicit target name: ".*")|(Hyperlink target ".*" is not referenced)

This comment has been minimized.

@humitos

humitos Mar 19, 2018

Member

By ignoring this now that are false positivies, we won't detect the ones that are true when we updated the docs :(

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 19, 2018

Optimization: is it possible to trigger this docs-lint env _only when there are changes in the docs/ directory?

I'm not sure if that would be possible, maybe a setting on travis? Since the linting is very quick, the heavy stuff comes from travis and installing the deps p:

@@ -33,7 +33,7 @@ Supported Top-Level Redirects
.. note:: These "implicit" redirects are supported for legacy reasons.
We will not be adding support for any more magic redirects.
If you want additional redirects,
they should live at a prefix like :ref:`page-redirect`
they should live at a prefix like `Redirecting to a Page`_

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Why did you change these ones?

Using ref is advised over standard reStructuredText links to sections (like Section title_) because it works across files, when section headings are changed, and for all builders that support cross-references.

from http://www.sphinx-doc.org/es/stable/markup/inline.html#role-ref

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

mmm, well I don't like the idea of exposing a lot of links as global refs and more when they aren't used on the global scope, but I will revert those if you consider.

This comment has been minimized.

@ericholscher

ericholscher May 24, 2018

Member

I wonder if we can make a Sphinx extension that automatically ads each section heading as a global ref, prefixed by pagename or similar? That way you wouldn't need to declare all the refs, but still be able to reference them. That's definitely for another ticket/PR though.

This comment has been minimized.

This comment has been minimized.

@ericholscher

ericholscher May 24, 2018

Member

Hmm, looks like those are still for explicit section refs, not implicitly creating them, ignore me :)

This comment has been minimized.

@ericholscher

ericholscher May 24, 2018

Member

Found it: http://www.sphinx-doc.org/en/stable/ext/autosectionlabel.html -- it's already built into Sphinx :)

This comment has been minimized.

@humitos

humitos May 24, 2018

Member

Like it!

@@ -1,5 +1,3 @@
.. _contributing-to-read-the-docs:

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Be careful when removing these ones. This one in particular, is used at https://github.com/rtfd/readthedocs.org/pull/3624/files#diff-c7f13470cacdf0634e185d4313d02a12R55 (L57 in design.rst) so that link is probably dead after removing this.

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

Ups, I just missed that one. I usually do a grep to search those references.

@@ -1,5 +1,3 @@
.. _`Localization of Documentation`:

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

This one is used in faq.rst

This comment has been minimized.

@stsewd
@humitos

This looks good and very close to merge, from my point of view.

I'm marking this as "Request changes" because there are some internal links that will be broken with these modifications.

We will need to be sure that each link that we modify still works before merging. You can check that by building the documentation by yourself in your local instance:

cd docs
make html
firefox _build/htmls/index.html
@stsewd

This comment has been minimized.

Member

stsewd commented Mar 23, 2018

@humitos done, just one link was broken, the others were properly modified. By the way I use sphinx-autobuild for building the docs on dev.

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 11, 2018

I just update this branch to the current master and fix an issue with some docs from upstream.

We only need to agree on using :ref: role for internal referencing or use the auto generated references from the titles.

@stsewd

This comment has been minimized.

Member

stsewd commented May 23, 2018

I just rebased and solved others conflicts from upstream, this should be ready to go, also I checked that all links work properly.

@stsewd

This comment has been minimized.

Member

stsewd commented May 23, 2018

Also, I found this target on the docs' Makefile make livehtml

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

@stsewd yea, that lets you auto-rebuild the docs if you have sphinx-autobuild installed.

@ericholscher

Great changes. We should write a blog post about this, so that other folks in the community might be able to benefit from the knowledge here.

/cc @davidfischer, can we add that to our editorial calendar, and I can help write it up w/ Santos' review perhaps.

@@ -33,7 +33,7 @@ Supported Top-Level Redirects
.. note:: These "implicit" redirects are supported for legacy reasons.
We will not be adding support for any more magic redirects.
If you want additional redirects,
they should live at a prefix like :ref:`page-redirect`
they should live at a prefix like `Redirecting to a Page`_

This comment has been minimized.

@ericholscher

ericholscher May 24, 2018

Member

I wonder if we can make a Sphinx extension that automatically ads each section heading as a global ref, prefixed by pagename or similar? That way you wouldn't need to declare all the refs, but still be able to reference them. That's definitely for another ticket/PR though.

@ericholscher ericholscher merged commit ea0c473 into rtfd:master May 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:add-rstcheck-to-ci branch May 24, 2018

@humitos

This comment has been minimized.

Member

humitos commented May 24, 2018

Goooood work! :)

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