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

Merged
merged 31 commits into from May 24, 2018
Merged

Add rstcheck to CI #3624

merged 31 commits into from May 24, 2018

Conversation

@stsewd
Copy link
Member

@stsewd 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`
Copy link
Member Author

@stsewd stsewd Feb 16, 2018

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?

Copy link
Member Author

@stsewd stsewd Feb 16, 2018

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

Copy link
Member

@humitos humitos Feb 20, 2018

It's already released, 3.2.

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

@stsewd
Copy link
Member Author

@stsewd 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
Copy link
Member Author

@stsewd 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
Copy link
Member

@ericholscher ericholscher commented Feb 18, 2018

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

@stsewd
Copy link
Member Author

@stsewd 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
Copy link
Member

@humitos 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
Copy link
Member Author

@stsewd 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`'
Copy link
Member

@humitos humitos Feb 20, 2018

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

@stsewd stsewd force-pushed the add-rstcheck-to-ci branch from 1886d28 to 98cd3ac Feb 21, 2018
tox.ini Outdated
commands =
sphinx-build -b html -d {envtmpdir}/doctrees . {envtmpdir}/html

[testenv:docs-lint]
deps = -r{toxinidir}/requirements/lint.txt
Copy link
Member

@humitos humitos Feb 21, 2018

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

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

Copy link
Member Author

@stsewd stsewd Feb 21, 2018

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

Copy link
Member

@humitos humitos Feb 21, 2018

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

@@ -0,0 +1,2 @@
-r pip.txt
Copy link
Member Author

@stsewd stsewd Feb 21, 2018

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

Copy link
Member

@humitos humitos Feb 21, 2018

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)

Copy link
Member

@humitos humitos Feb 21, 2018

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
Copy link
Member

@humitos humitos Feb 21, 2018

Py3? :)

@stsewd
Copy link
Member Author

@stsewd 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 stsewd force-pushed the add-rstcheck-to-ci branch from 68d9cd1 to efa5e7b Mar 17, 2018
@stsewd
Copy link
Member Author

@stsewd 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
Copy link
Member Author

@stsewd stsewd commented Mar 17, 2018

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

@stsewd
Copy link
Member Author

@stsewd 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)
Copy link
Member Author

@stsewd stsewd Mar 17, 2018

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

Copy link
Member

@humitos humitos Mar 19, 2018

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

Copy link
Member Author

@stsewd stsewd Mar 19, 2018

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

Copy link
Member Author

@stsewd stsewd Mar 19, 2018

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.

Copy link
Member

@humitos humitos Mar 23, 2018

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

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

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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

Copy link
Member

@humitos humitos left a comment

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`__.
Copy link
Member

@humitos humitos Mar 19, 2018

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

Copy link
Member

@humitos humitos Mar 19, 2018

(the same for the other changes in this file)

Copy link
Member Author

@stsewd stsewd Mar 19, 2018

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.

Copy link
Member

@humitos humitos Mar 23, 2018

`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.

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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)

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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)
Copy link
Member

@humitos humitos Mar 19, 2018

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

@stsewd
Copy link
Member Author

@stsewd 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`_
Copy link
Member

@humitos humitos Mar 23, 2018

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

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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.

Copy link
Member

@ericholscher ericholscher May 24, 2018

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.

Copy link
Member

@ericholscher ericholscher May 24, 2018

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

Copy link
Member

@ericholscher ericholscher May 24, 2018

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

Copy link
Member

@humitos humitos May 24, 2018

Like it!

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

@humitos humitos Mar 23, 2018

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.

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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

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

@humitos humitos Mar 23, 2018

This one is used in faq.rst

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

Yeah, and was changed to use the doc directive as rtd recommends

https://github.com/rtfd/readthedocs.org/pull/3624/files#diff-c6f0a0722c6a2f86277535d7bcec7f8cR167

Copy link
Member

@humitos humitos left a comment

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
Copy link
Member Author

@stsewd 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 stsewd force-pushed the add-rstcheck-to-ci branch from fbceb5f to d6292bc May 23, 2018
@stsewd
Copy link
Member Author

@stsewd 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
Copy link
Member Author

@stsewd stsewd commented May 23, 2018

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

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 24, 2018

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

Copy link
Member

@ericholscher ericholscher left a comment

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`_
Copy link
Member

@ericholscher ericholscher May 24, 2018

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 readthedocs:master May 24, 2018
1 check passed
@stsewd stsewd deleted the add-rstcheck-to-ci branch May 24, 2018
@humitos
Copy link
Member

@humitos 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants