WIP: add crossrefs checker for documentation linting

[hoefling]

This partly addresses #9081.

This PR adds a new no-op Sphinx builder named crossrefcheck that checks whether a hardcoded URL in the text can be replaced with a crossreference from one of the inventories configured in intersphinx_mapping. If yes, crossrefcheck will emit a warning like this:

pytest/doc/en/reference/reference.rst:127: WARNING: hardcoded link 'https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertAlmostEqual' could be replaced by a cross-reference to 'python' inventory (try using ':py:meth:`unittest.TestCase.assertAlmostEqual`' instead)

If a replacement suggestion is printed, the hardcoded URL can be safely replaced with that suggestion.

Usage

$ sphinx-build -b crossrefcheck doc/en/ _build

Caveats

The builder is not able to catch the URLs for the internal docs. This is because no information is known about the URL the docs will be hosted at. Thus, if links to https://docs.pytest.org should be checked as well, it is best to extend intersphinx_mapping. Example:

 intersphinx_mapping = {
    ...
    "pytest-latest": ("https://docs.pytest.org/en/latest", None),
    "pytest-stable": ("https://docs.pytest.org/en/stable", None),
    "pytest-6": ("https://docs.pytest.org/en/6.2.x", None),
 }

If a URL is reported without a suggestion, it usually indicates that either:

1. a link points to a section or paragraph that doesn't have a Sphinx reference defined - the external docs should be updated then,
2. or the reference was changed, so the link is outdated (but still valid!). In this case, it's usually best to check the docs source to find the new correct ref name.

[hoefling]

@Zac-HD will do a proper PR out of this in the next days.

[Zac-HD]

I'd also like to consider proposing this upstream as sphinx.ext.crossrefcheck - it's already pretty generic, and that would make it much easier to pick up in other projects across and beyond pytest-dev 😁

The only changes involved would be to drop the pytest-specific logging line - you can add your own docs to the intersphinx mapping. I also think using the same message for internal refs is fine, but we could also use the project config key to work out which (if any) is internal. It would be easy to add similar logic for extlinks, at least if we also added the internal key to generated reference nodes.

Thoughts?

[hoefling]

Glad that you liked it! It's a great idea to keep the internal inventory in intersphinx_mapping. I also agree on unifying the warnings - not sure why I did distinct between both in the first place. I'm not sure whether sphinx devs are willing to maintain another builder, but worth a shot IMO. However, now I remember that I also wanted to add a replacement suggestion to the warnings, e.g.

file.rst:1:WARNING: hardcoded link 'https://docs.pytest.org/en/stable/reference.html#pytest.mark.xfail' could be replaced by a cross-reference to 'pytest' inventory. Possible replacement could be ':func:`pytest.mark.xfail`'

I have another script for pre-commit somewhere that does that, so I would like to integrate that into the builder first and then see what sphinx devs say to the combined impl.

[Zac-HD]

However, now I remember that I also wanted to add a replacement suggestion to the warnings

If we're going that far... how hard would it be to just implement the suggestion? For safety maybe only if you pass a --inplace flag, but if an autoformatter can do it that's a lot nicer than a linter.

[nicoddemus]

I have another script for pre-commit somewhere that does that, so I would like to integrate that into the builder first and then see what sphinx devs say to the combined impl.

Awesome!

I'm not sure whether sphinx devs are willing to maintain another builder, but worth a shot IMO.

If that's the case they don't want to include that checker in Sphinx directly, consider making this a standalone package on PyPI? This would allow to reuse it nicely. 👍

[hoefling]

Done with the initial impl that also includes suggestions, will now prepare a PR for https://github.com/sphinx-doc/sphinx. Also updated the PR description, just for the sake of completeness.

@Zac-HD

If we're going that far... how hard would it be to just implement the suggestion? For safety maybe only if you pass a --inplace flag, but if an autoformatter can do it that's a lot nicer than a linter.

This would be awesome, but I don't have any clue on how to implement this properly. I researched the inplace modification of ReST files with Sphinx when implementing my first doc checkers two years ago. Unfortunately, Sphinx doesn't offer anything that can parse ReST and write ReST back (something like what libcst does for Python sources). I recall finding a third-party ReST writer, but it was buggy and couldn't at that point handle the pretty extensive docs I wanted to refurbish. Maybe it got better now, but I can't find it anymore 😞

Another issue is that it's quite hard to trace the ReST source in docstrings - this is something not resolved with the impl in this PR as well. When a hardcoded URL is dropped in a docstring, Sphinx will display a wrong and totally misleading line number to the autodoc's directive in ReST file. E.g. for pytest, this will always be reference.rst, not the Python module.

[hoefling]

The upstream PR is at sphinx-doc/sphinx#9626.

[hoefling]

Closing this since the Sphinx maintainers seem to agree on adding the feature to Sphinx directly.