Skip to content

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

Closed
wants to merge 15 commits into from

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Oct 21, 2020

(Split from #1570.)

This PR adds two reStructuredText lint rules and runs them on the CI using GitHub Actions. Example run.

  1. Detect single backticks, which should be double in RST for inline code, e.g. stuff fixed in Fix RST backticks #1554.

  2. 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:602:    `single-quoted' or ``double-quoted''
pep-0505.rst:245:    Total None-coalescing `if` blocks: 449
pep-0505.rst:246:    Total [possible] None-coalescing `or`: 120
pep-0505.rst:248:    Total Safe navigation `and`: 13
pep-0505.rst:249:    Total Safe navigation `if` blocks: 61
pep-0550.rst:540:    g = gen()  # gen_LC is created for the generator object `g`
pep-0550.rst:763:        # This class is what the `gen_series()` generator can
pep-0550.rst:772:            # Otherwise `var.set(10)` in the `_init` method
pep-0550.rst:857:        # in the `Context Variables` section.
  • 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.

@hugovk hugovk force-pushed the lint-rsk-backticks-only branch from d9207ad to 8b8057c Compare February 1, 2021 08:00
@hugovk
Copy link
Member Author

hugovk commented Feb 1, 2021

Rebased and fixed new broken backticks in 6 more PEPs.

@gvanrossum
Copy link
Member

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.

@hugovk
Copy link
Member Author

hugovk commented Feb 3, 2021

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?

@gvanrossum
Copy link
Member

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.

@hugovk
Copy link
Member Author

hugovk commented Feb 5, 2021

Please see #1797 which has no changes to code blocks, and no linter.

@gvanrossum
Copy link
Member

Let’s close this. When the linter is grown up we can add it to CI.

@gvanrossum gvanrossum closed this Feb 5, 2021
@gvanrossum
Copy link
Member

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.

@hugovk
Copy link
Member Author

hugovk commented Feb 7, 2021

Please see PR #1802:

The rst-backticks linter has been updated and now skips code blocks (lines beginning with four spaces).

@hugovk hugovk deleted the lint-rsk-backticks-only branch February 7, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants