Skip to content

Lint RST inline code on GitHub Actions #1570

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Lint RST inline code on GitHub Actions #1570

wants to merge 3 commits into from

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Aug 15, 2020

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-0567.rst:159:    # Get the value of `var`.
pep-0572.rst:1286:        # `a` is local to `f`, but remains unbound
pep-0572.rst:1301:    `a` is not yet bound

  • 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.

  • The other three are from comments in code. Because it's (indented) code, no extra formatting is added in the rendered monospace output. so these could be changed to either double backticks or no backticks from a formatting point of view.

Now, there's no way to exclude single lines from the linter, so the whole file needs to be excluded.

Question

Should the single backticks be changed in the latter three code comments?

@encukou
Copy link
Member

encukou commented Aug 18, 2020

Could this be done in the build itself?
That way the build would always fail if there's an issue. With a linter, I'd see only failures after submitting a PR (or I'd have to do some additional setup locally, but it's not obvious how to do that).

It looks like single backticks are customizable; can they be customized to raise errors?

@hugovk
Copy link
Member Author

hugovk commented Aug 18, 2020

Could this be done in the build itself?
That way the build would always fail if there's an issue. With a linter, I'd see only failures after submitting a PR (or I'd have to do some additional setup locally, but it's not obvious how to do that).

By build, do you mean the Travis CI build? Yes, it could be easily added there. I chose to put it in a separate thing so if there's a problem, it obvious if it's from the build or the lint. For example:

But happy to move it to Travis CI if that's preferred.

You can also enable GitHub Actions (and Travis CI) for your fork, so you can see failures before submitting a PR.

Yes, it's possible to check locally, in a couple of ways.

# One off
pip install -U pre-commit

# Lint all the RST files
make lint

So you could do stuff like make lint && make -j 4

# One off
pip install -U pre-commit
pre-commit install

# Commit stuff as normal, it'll run the lint (if RST changed)
# and fail the commit if there's a problem
git commit -m "PEP XXX: etc"

It looks like single backticks are customizable; can they be customized to raise errors?

Nice idea, I'm not familiar enough with RST for this.

@brettcannon
Copy link
Member

We can also move the build to GitHub Actions; Travis is not special in this case.

@hugovk
Copy link
Member Author

hugovk commented Aug 19, 2020

I've added a GitHub Actions workflow to the build and deploy, like Travis CI.

Notes:

  • Travis CI builds against 3.7 and 3.7-dev. I only added 3.8 to GHA. Should it build against a range? For example, we can do 3.6-3.8 and 3.9-dev.

  • It will only deploy for python/peps and master

  • The deploy will need credentials. I guess there are some at
    https://travis-ci.com/github/python/peps? They will need to be added at https://github.com/python/peps/settings/secrets

  • I've left Travis CI in place for now. I'd suggest confirming the deploy works first from GHA, then removing from Travis in a followup PR.

  • (I also noticed that on Travis, it's deploying for both 3.7 and 3.7-dev. It probably only needs doing from a single job.)

  • Nice bonus: make -j$(nproc) is quicker on GHA (~80s) than Travis (~110s)

@hugovk
Copy link
Member Author

hugovk commented Oct 20, 2020

Would it help if I split this PR?

@hugovk hugovk mentioned this pull request Oct 20, 2020
Merged
@brettcannon
Copy link
Member

@hugovk you mean split the Travis -> GHA work from the linting part? Yeah, it would probably help since that removes the worry of whether the linting is what we want from whether we want to move off of Travis.

pradyunsg
pradyunsg reviewed Oct 21, 2020
View reviewed changes
.github/workflows/build.yml
)

run: |
bash deploy.bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually triggering a deployment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but only for the python/peps repo and master branch (like .travis.yml).

As a demo, here's another branch that allows a deploy for my fork and that branch:

The deploy is triggered (and fails, as expected due to lack of credentials in my repo):

This was referenced Oct 21, 2020
Closed
Merged
@hugovk
Copy link
Member Author

hugovk commented Oct 21, 2020

@hugovk you mean split the Travis -> GHA work from the linting part? Yeah, it would probably help since that removes the worry of whether the linting is what we want from whether we want to move off of Travis.

Replaced by separate PRs:

@hugovk hugovk closed this Oct 21, 2020
@hugovk hugovk deleted the lint-rsk-backticks branch February 9, 2021 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pradyunsg pradyunsg pradyunsg left review comments

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hugovk @encukou @brettcannon @pradyunsg @the-knights-who-say-ni