-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Replace doclinter with sphinx-lint #10389
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sphinx-lint
at a point where we can add it to CI for core Sphinx?
A
Yes, multiple projects already does it (cpython, pandas, sympy, ...) but I would recommend not having it mandatory yet for Sphinx, as it's typically here that we can find surprising things like rst-in-rst ;) Do you want me to add the github workflow in the same PR? |
Please, currently Sphinx uses a custom linter via tox: Lines 88 to 95 in 5eeeb9c
I think using a standard tool would be beneficial. A |
Updated the Makefile:
Updated tox.ini:
And Github Actions side I just bumped the Python version from 3.6 to 3.7 in the lint workflow, because sphinx-lint requires it. |
CI fails due to psf/requests#6140. |
The CI could probably be fixed by changing https://docs.python-requests.org to https://requests.readthedocs.io, I don't know if there's a precedent on how to handle this? Maybe just wait a few hours. I'm following upstream's issue. |
@JulienPalard I'd advise moving the domain to https://requests.readthedocs.io if that's acceptable. That will be the primary domain going forward as there isn't a clear path to Kenneth retrieving the python-requests.org domain. |
Thanks Nate, we went ahead and did this in #10484. Thanks for confirming https://requests.readthedocs.io/ will be the main domain going ahead. A |
I closed & re-opened but just realised the fix won't be in this branch so it'll most likely fail again! Sorry for the noise. A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note, looks good otherwise.
A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you for developing great tool!
@JulienPalard it is failing on the below, which is valid reST (rendered). I think check_missing_space_after_literal needs to be updated to ignore The version of Sphinx used to build represented as a tuple of five elements.
For Sphinx version 3.5.1 beta 3 this would be ``(3, 5, 1, 'beta', 3)``.
The fourth element can be one of: ``alpha``, ``beta``, ``rc``, ``final``.
``final`` always has 0 as the last element. For now I will disable that check and merge if CI passes, as it is a good improvement. A |
Thanks @JulienPalard! A |
Can't reproduce: |
Subject: Just ran sphinx-lint on Sphinx own doc.
Unsurprisingly I found more issues in
sphinx-lint
than inSphinx doc
while doing so ;-)But it found a real one:
so I'm happy. Other things were trailing whitespaces, and so on.