Describe running lint and spellcheck locally in README/Contributing guide#2338

CAM-Gerlach · 2022-02-17T03:44:11Z

Adds a description of how to run the linting and spellcheck locally to the README and Contributing Guide and an admonition against mass spellcheck PRs, as well as updates the codeowners for the linting-related infra.

The contents of the README vs. Contributing Guide are a touch disjointed, and the content in the former pertaining to the Sphinx builds is somewhat out of date and duplicated/redundant to that in the docs directory (and in the --help of build.py), but I didn't touch that here for now.

Fixes#2265

AA-Turner

Looks good overall.

A general, although vague comment is that the new README text feels too verbose. I'm not sure how much detail is needed here? Should we just list commands that might be needed and redirect to CONTRIBUTING.rst?

A

.github/CODEOWNERS

AA-Turner · 2022-02-17T17:05:15Z

README.rst

-The default Make target generates HTML for all PEPs.
-
-If you don't have Make, use the ``pep2html.py`` script directly.
+Avoid committing changes with reStructuredText syntax errors that cause PEP


I prefer the strict admonition of the prior text ("Do not commit ...") -- it is much clearer than "avoid"

I changed it to "Please don't" to still carry the same weight, but be a little less harsh. I also fixed some editing typos I spotted in my changes.

"please" is almost always an unneeded word in documents like this -- as Hugo said readers tend to scan, so being nice is somewhat wasted!
A

It may be strictly semantically unnecessary, but it just sounds unnecessarily harsh and intimidating to be telling new contributors "Don't commit changes with syntax errors", like telling them "Don't make mistakes!"

I think it is a reasonable admonition to make though -- those who can directly commit to the repo (core devs and editors) should be told not to make mistakes, and the PR checks will catch people unfamiliar with the syntax. I saw that as an admonition aimed at the former group (emphasis on commit) rather than the latter group (emphasis on mistakes).
It is, however, one word. This converstation has used many more than that so I'll let you flip a coin or something (I go heads!)
A

AA-Turner · 2022-02-17T17:10:15Z

CONTRIBUTING.rst

+Run pre-commit linting locally
+------------------------------
+
+If you'd like, you can run the project's basic linting suite locally,


"the project's" feels odd verbiage for the PEPs repo -- perhaps out of pomposity, though

I changed it to "this repo's"—is that better?

CAM-Gerlach · 2022-02-17T19:58:20Z

A general, although vague comment is that the new README text feels too verbose. I'm not sure how much detail is needed here? Should we just list commands that might be needed and redirect to CONTRIBUTING.rst?

As I hint above, I agree that a lot of the details should be moved out of there to the contributing guide, or elided entirely since they are already discussed in your docs and build.py --help, but that's a better topic for another issue/PR (and one I was considering tackling in the future), since that requires more substantial changes to the rest of the README and contributing guide.

hugovk

Looks good.

A benefit of having stuff in the contributing page is it's linked to when you open a PR.

A downside of having stuff in the contributing page is you might not see it until you're about to open a PR, after you've done the work!

Perhaps some of the prose descriptions of commands could refactored to this form:

# To do x: command_that_does_x # Or to do y: command_that_does_y

Benefit: easier to scan, and copy and paste.
Downside: less suitable when more detail is needed.

CONTRIBUTING.rst

hugovk · 2022-02-17T19:52:22Z

CONTRIBUTING.rst

+``pre-commit run --hook-stage manual codespell``, or against all files with
+``pre-commit run --all-files --hook-stage manual codespell``.
+
+**Note**: While fixing spelling mistakes as part of more substantive


Perhaps even make this a subheading so we can easily link to it?

Hmm, I guess we could, but after playing around with it I'm not sure it fits well as a full section, nor what a good title for it would be. We can just link the PEP spelling heading, no? Its not like such PRs have been that frequent here...

Sure, that'll be fine.

Actually, after some discussion on #2350 , I think it would be a good idea to make this a top-level section to discuss what our general policy for changing older PEPs, what contributions are helpful, and which are likely to be declined, I'll modify it accordingly.

Err, further revision—after I started drafting text for this section, given this is a broader policy than what was originally envisioned here and might require additional discussion, I think it would be best to keep this PR in scope and add this in an immediate follow-up. I also drafted some other updates for other out of date parts of the contributing guide and readme which can also be handled separately (in this case independently, since they don't conflict with either of these changes).

.github/CODEOWNERS

README.rst

…feedback Co-authored-by: Hugo van Kemenade <[email protected]>

CAM-Gerlach · 2022-02-18T03:01:37Z

Perhaps some of the prose descriptions of commands could refactored to this form

I would normally do that, but I wasn't sure how the GitHub reST render would handle them; I know previously it handled reST pretty poorly, and was too lazy to test. But it looks like it works just fine, so I've gone ahead with that, thanks.

hugovk

Thank you!

encukou · 2022-02-18T09:58:24Z

As far as I know, this will download and run code from the internet, and there are external people/systems/processes that I need to (implicitly) trust if I choose to run this. Is that right? If so, I think it should at least be noted.

CAM-Gerlach · 2022-02-18T21:23:27Z

As far as I know, this will download and run code from the internet, and there are external people/systems/processes that I need to (implicitly) trust if I choose to run this. Is that right? If so, I think it should at least be noted.

Sure, but so will the other make commands that automatically install the requirements, etc., and the other commands we specify for doing the same and this repo itself must be downloaded and run from the internet with a substantial amount of executable code of its own. On the converse, our pre-commit config doesn't install any third-party executable hooks, only declarative regexes (from here or in pre-commit's official repository) and a few hook scripts in pre-commit's built-in official pre-commit-hooks repo, which is no more vulnerable to compromise than pre-commit's executable code, like any other dependency we install.

In fact, by design, pre-commit only downloads hooks using commit hashes and tags checked into the repo, so users are getting the exact same tested code that runs both locally when originally added/updated and on the CIs on every PRs and push, and it is thus less vulnerable to undetected compromise than the the various unpinned dependencies (and their dependencies) that can install an arbitrary new, untested version on a user's local machine without any deliberate action or testing on our part.

But maybe there's something I'm missing here?

AA-Turner · 2022-02-19T02:52:50Z

But maybe there's something I'm missing here?

This comment is assuming @encukou is talking about the Makefile // make commands. If so, the only other command that installs software is make venv. A line that notes "make blah will also install pre-commit if it is not found" I think would be reasonable.

A

README.rst

CAM-Gerlach · 2022-02-19T03:16:38Z

A line that notes "make blah will also install pre-commit if it is not found" I think would be reasonable.

Indeed, and I already have exactly that, in the line following the mention of said make lint command:

This will install pre-commit in the current virtual environment if it isn't already, so make sure you've activated the environment you want it to use before running this command.

AA-Turner · 2022-02-19T03:18:05Z

I already have exactly that, in the line following

You presuppose that I can read ;-) My point is moot then though, sorry for the bother!

A

AA-Turner · 2022-02-22T00:06:26Z

Thanks!
A

CAM-Gerlach added 4 commits February 16, 2022 20:21

Add and consolidate codeowners for linting infrastructure
6dfa2b3

Update Makefile to use alias for lint and spellcheck jobs
945b213

Update the README to provide more detail on checking PEP rendering
efb7b46

Add explaination for how to use pre-commit in Contributing Guide
250f98e

CAM-Gerlach self-assigned this Feb 17, 2022

CAM-Gerlach requested review from a team and AA-Turner as code owners February 17, 2022 03:44

the-knights-who-say-ni added the CLA signed label Feb 17, 2022

CAM-Gerlach requested a review from hugovk February 17, 2022 03:44

AA-Turner reviewed Feb 17, 2022
View reviewed changes

CAM-Gerlach force-pushed the add-lint-to-contrib-guide branch from c407f54 to 6cc3ff0Compare February 17, 2022 19:56

CAM-Gerlach requested a review from AA-Turner February 17, 2022 19:58

hugovk reviewed Feb 17, 2022
View reviewed changes

CAM-Gerlachand others added 2 commits February 17, 2022 20:44

Add section to Contributing Guide on how to check PEP spelling
b88d052

Improve readme and contributing guide sections further with reviewer …
9ddc9a8
…feedback Co-authored-by: Hugo van Kemenade <[email protected]>

CAM-Gerlach force-pushed the add-lint-to-contrib-guide branch from 7f47462 to 9ddc9a8Compare February 18, 2022 02:46

Format check, lint & spelling commands in Readme/Contribguide as blocks
6ec177e

JelleZijlstra approved these changes Feb 18, 2022
View reviewed changes

hugovk approved these changes Feb 18, 2022
View reviewed changes

AA-Turner reviewed Feb 19, 2022
View reviewed changes

README.rstShow resolvedHide resolved

CAM-Gerlach mentioned this pull request Feb 21, 2022
PEP 544: Replace broken link to Zope documentation #2350
Merged

CAM-Gerlach marked this pull request as draft February 21, 2022 20:02

CAM-Gerlach marked this pull request as ready for review February 21, 2022 20:50

CAM-Gerlach requested a review from AA-Turner February 21, 2022 20:50

CAM-Gerlach mentioned this pull request Feb 21, 2022
Update, copyedit and format Readme and Contributing Guide #2352
Merged

AA-Turner merged commit 6064e22 into python:mainFeb 22, 2022

CAM-Gerlach mentioned this pull request Mar 4, 2022
Meta: Clearly document current guidance for changing existing PEPs #2378
Merged
5 tasks

erlend-aasland mentioned this pull request May 13, 2022
pep8/greppable exception messages erlend-aasland/peps#1
Closed

erlend-aasland mentioned this pull request Jun 27, 2022
pep 687/mark as accepted erlend-aasland/peps#2
Closed

Uh oh!

Describe running lint and spellcheck locally in README/Contributing guide#2338

Describe running lint and spellcheck locally in README/Contributing guide #2338

Uh oh!

Conversation

CAM-Gerlach commented Feb 17, 2022

Uh oh!

AA-Turner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Feb 17, 2022

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CAM-GerlachFeb 21, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CAM-Gerlach commented Feb 18, 2022

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

encukou commented Feb 18, 2022

Uh oh!

CAM-Gerlach commented Feb 18, 2022

Uh oh!

AA-Turner commented Feb 19, 2022

Uh oh!

Uh oh!

CAM-Gerlach commented Feb 19, 2022

Uh oh!

AA-Turner commented Feb 19, 2022

Uh oh!

AA-Turner commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CAM-GerlachFeb 21, 2022•
edited
Loading