Lint: Implement check for spaces, not tabs per PEP 12 #2379
Conversation
CAM-Gerlach
changed the title
| @@ -70,6 +70,12 @@ repos: | |||
| # Local checks for PEP headers and more | |||
| - repo: local | |||
| hooks: | |||
| - id: check-no-tabs | |||
There was a problem hiding this comment.
We're using the remove-tabs hook (https://pre-commit.com/hooks) in another project that will not just flag tabs but also replace them with four spaces.
Shall we use that?
There was a problem hiding this comment.
Yeah, that hook is what I use in my other projects as well. I didn't use that here since unlike on those projects, AFAIK most authors/editors don't run the hooks locally and instead its mostly run on CI, where the replacement doesn't matter, and it maintains the status quo of all the hooks we use being either first party or local, rather than on code from individual third party repos.
There was a problem hiding this comment.
I've a strong preference for warn+fix over only warn.
For those authors/editors don't run it locally, it's all the same.
But autofix is really useful for those of us who do run locally.
I don't see a need to restrict to only first party or local, and we already have third-party https://github.com/codespell-project/codespell.
And infact we could add https://pre-commit.ci to autofix PRs. I'm also using this for other repos and I find it very useful: no need for a reviewer to ask the author to make changes, then wait for them to make them, and check they made them properly.
We also have a number autofixers from https://github.com/pre-commit/pre-commit-hooks as well which could benefit from https://pre-commit.ci.
There's precedence in this org, https://github.com/python/typeshed already uses https://pre-commit.ci (added in python/typeshed#5552).
@JelleZijlstra: how's your experience been with https://pre-commit.ci/ on https://github.com/python/typeshed?
There was a problem hiding this comment.
It's been great on typeshed and saved a lot of time. It doesn't seem as important on this repo though because far fewer PRs run into CI issues that pre-commit could fix.
There was a problem hiding this comment.
I've a strong preference for warn+fix over only warn. For those authors/editors don't run it locally, it's all the same. But autofix is really useful for those of us who do run locally.
I strongly do so as well, and rely heavily on this in many other repos I maintain, but this is a bit of a special case. Based on this repo's history, this check should trigger quite rarely (one table in one series of PEPs as of now), we currently only rely on first or second-party hooks for checks that aren't manual-only, and this repo has (AFAIK) much more limited use of the hooks locally than any of the others I work on.
I don't see a need to restrict to only first party or local, and we already have third-party https://github.com/codespell-project/codespell.
That's true, but it isn't installed and run via pre-commit install or on CI; it must be explicitly requested manually by the user (typically after reading our docs explaining how to do so).
And infact we could add https://pre-commit.ci/ to autofix PRs. I'm also using this for other repos and I find it very useful: no need for a reviewer to ask the author to make changes, then wait for them to make them, and check they made them properly.
Pre-commit CI has some pretty serious limitations, particularly as to what hooks it can run; see pre-commit/action#118 . Furthermore, it is very unlikely to ever trigger (see above and next item), and I'd rather not spend this repo's limited churn budget adding it, especially when it is pretty strained already.
We also have a number autofixers from https://github.com/pre-commit/pre-commit-hooks as well which could benefit from https://pre-commit.ci/.
We only have a grand total of three hooks that offer autofixing:
mixed-line-ending, which is autofixed locally by our.gitattributesconfig (its only there as a backstop for the local checkout)fix-byte-order-marker, which again is there as a backstop and I don't think I've actually seen trigger on any repo I managefile-contents-sorter, which only affects the Codespell ignore words list which realistically is only going to be updated by you or me, who both presumably have pre-commit locally.
As @JelleZijlstra also alluded to, these autofixers should never actually trigger on CI (as opposed to locally), and I don't think I've seen a single PR on this repo on which they have (and even the other checks trigger pretty rarely). As mentioned above, the tab check is unlikely to trigger more than very rarely, and is a few seconds total to fix in any editor. So in total, it is likely that even 10 years from now it would never pay back even the time we've spent haggling about it 😆
There was a problem hiding this comment.
Okay, this is still a good improvement, thanks!
In a comment on #2374 , @hugovk mentioned that while PEP 12 unequivocally specifies that tabs should not be used in PEPs and spaces used instead, we don't currently have a linting check for this (which I thought we did). This allowed tabs to slip into list of core developers present in PEPs 8100-8103. Therefore, we convert them to spaces, in conformance with PEP 12 and the rest of the PEPs, and implement a simple linting check in case any sneak by in the future.