-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Lint RST inline code on GitHub Actions #1668
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
fac5cd0
to
d9207ad
Compare
d9207ad
to
8b8057c
Compare
Rebased and fixed new broken backticks in 6 more PEPs. |
So I'm having a hard time understanding why we need to touch code blocks at all. AFAICT backticks there are valid (they certainly work in practice). Is this just a deficiency in the linter? I'd be happy to accept changes that fix actually incorrect uses of backticks outside of code blocks (you found several of those), but for the code blocks I think the linter should be fixed first, or you have to convince me that backticks in code blocks are invalid ReST. |
Yes, it's a deficiency in the linter. Looks like backticks in code blocks are valid ReST, although no extra extra formatting is applied to them. So they can be safely changed, but that's a style choice and not a validity thing. Anyway, I'll look into updating the linter to ignore code blocks. It's built with a regex not full ReST parser. Would it be reasonable to ignore backticks on lines beginning with four spaces? |
I'm happy that you occasionally run the linter and then fix real bugs like PEP 621 or PEP 635. Can you make those a separate PR? But I don't want to see any code samples changed to satisfy the linter. And obviously this linter needs to work better before we can accept running it on each PR. |
Please see #1797 which has no changes to code blocks, and no linter. |
Let’s close this. When the linter is grown up we can add it to CI. |
As a compromise could we add a script to the repo that devs can run to lint one pep? I know I would use it. |
Please see PR #1802: The |
(Split from #1570.)
This PR adds two reStructuredText lint rules and runs them on the CI using GitHub Actions. Example run.
Detect single backticks, which should be double in RST for inline code, e.g. stuff fixed in Fix RST backticks #1554.
Detect inline code touching normal text, e.g. stuff fixed in Fix incorrect backticks #1560.
This uses the pre-commit tool for linting. It's possible, but completely optional, to use it locally and it'll check your changes when you're committing, and fail before commit if something is found.
Optional local set up (run once):
pip install -U pre-commit && pre-commit install
Then make changes and commit as normal:
git commit -m "PEP XXX: etc"
Lint all files:
pre-commit run --all-files # Or this does the same thing make lint
The very first run takes a little while to set things up, after that it's quick.
But again, local use of pre-commit is completely optional for those who aren't keen, and the CI will run the checks anyway.
The first lint rule finds some false positives:
pep-0012.rst is an example of what not to do! So that needs to stay.
pep-0505.rst is example output from a command, so (most likely) needs to stay.
There's no way to exclude single lines from the linter, so both these files have been excluded.
Backticks already in code: I've changed them to single quotes. Because it's already formatted with code, no extra formatting was added in the rendered monospace output, so single quotes can also be used for the emphasis.
After rebasing on
master
, this found a few other single backticks which should have been double backticks or links. I've fixed them.