-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Lint: Implement check for spaces, not tabs per PEP 12 #2379
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using the
remove-tabshook (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
That's true, but it isn't installed and run via
pre-commit installor on CI; it must be explicitly requested manually by the user (typically after reading our docs explaining how to do so).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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is still a good improvement, thanks!