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 linter configuration and check in the CI #4

Open
lieser opened this issue Feb 3, 2023 · 4 comments
Open

Add linter configuration and check in the CI #4

lieser opened this issue Feb 3, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@lieser
Copy link
Collaborator

lieser commented Feb 3, 2023

To enforce best practice and catch errors we may want to run a linter for reStructuredText.

The following linters seem to exists:

We should decide which linter we want to use and add a configuration for it. Also investigate if it makes sense to run both linters.

If we add a linter a CI needs to be added for it.

@lieser lieser added the enhancement New feature or request label Feb 3, 2023
@reneme
Copy link
Collaborator

reneme commented Mar 31, 2023

I played a bit with rstcheck and wasn't really convinced. It's supposed to have a Sphinx integration but that is "under heavy development and not yet ready for use" (1).

One can run it on the individual RST files and it even finds a few relevant issues. Though, about 90% are " Unknown target name" issues. I believe that's because it cannot deal with references that span multiple RST files. One could ignore those (with a config file) and at least use the "local" warnings.

Those are the warnings (on current cryptodoc) that I think are worth looking at:

cryptodoc/src/07_pubkey.rst:2: (INFO/1) Enumerated list start value not ordinal-1: "0" (ordinal 0)
cryptodoc/src/08_signatures.rst:124: (INFO/1) Duplicate implicit target name: "signature creation".
cryptodoc/src/08_signatures.rst:166: (INFO/1) Duplicate implicit target name: "signature verification".
cryptodoc/src/08_signatures.rst:224: (INFO/1) Duplicate implicit target name: "signature creation".
cryptodoc/src/08_signatures.rst:267: (INFO/1) Duplicate implicit target name: "signature verification".
cryptodoc/src/08_signatures.rst:310: (INFO/1) Duplicate implicit target name: "signature creation".
cryptodoc/src/08_signatures.rst:349: (INFO/1) Duplicate implicit target name: "signature verification".
cryptodoc/src/08_signatures.rst:388: (INFO/1) Duplicate implicit target name: "signature creation".
cryptodoc/src/08_signatures.rst:423: (INFO/1) Duplicate implicit target name: "signature verification".
cryptodoc/src/08_signatures.rst:460: (INFO/1) Duplicate implicit target name: "signature creation".
cryptodoc/src/08_signatures.rst:549: (INFO/1) Duplicate implicit target name: "signature creation".
cryptodoc/src/08_signatures.rst:588: (INFO/1) Duplicate implicit target name: "signature validation".
cryptodoc/src/90_bibliographie.rst:5: (INFO/1) Enumerated list start value not ordinal-1: "R" (ordinal 18)
cryptodoc/src/11_x509.rst:2: (INFO/1) Enumerated list start value not ordinal-1: "e" (ordinal 5)
Error! Issues detected.

@reneme
Copy link
Collaborator

reneme commented Mar 31, 2023

Similarly for doc8. It mainly complains about "line too long", which might be a good thing, and is configurable. The second-most complaint is about "unknown target name". I.e. links across RST files.
And that's it. No more findings in the current cryptodoc.

But that's not too surprising, as it mostly checks for whitespace-related quirks (e.g. carriage returns, trailing white space, new line before EOF, ...).

@lieser
Copy link
Collaborator Author

lieser commented Mar 31, 2023

Then I tried both a few weeks ago I was also not that convinced so much with the linters.

Like you I noticed that they don't work well with Sphinx's multiple RST files. And the suppression of warnings is also sub optimal.

A note about rstcheck: the new version 6 is not supported by the VS Code extension (https://docs.restructuredtext.net/articles/linter#rstcheck). But that alone should not prevent us from enabling it in the CI.

My opinion: both linters don't seem to bring as much as I hoped for. But probably still better than having none.

@reneme
Copy link
Collaborator

reneme commented Mar 31, 2023

My vote is at most to use rstcheck (with the incompatible checks disabled) and otherwise rely on the Sphinx build to find issues. I feel we won't get around a visual/manual inspection of the resulting documents with this tooling anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants