Describe running lint and spellcheck locally in README/Contributing guide

[CAM-Gerlach]

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

[AA-Turner]

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

[CAM-Gerlach]

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.

[AA-Turner]

"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

[CAM-Gerlach]

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!"

[AA-Turner]

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]

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

[CAM-Gerlach]

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

[CAM-Gerlach]

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.

[hugovk]

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

[CAM-Gerlach]

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

[hugovk]

Sure, that'll be fine.

[CAM-Gerlach]

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.

[CAM-Gerlach]

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

[CAM-Gerlach]

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]

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]

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]

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

[CAM-Gerlach]

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]

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]

Thanks!
A