Add more pre-commit hooks#339
Conversation
Hooks added: * check-merge-conflict - checks for files with merge conflict strings * check-toml - attempts to load all toml files to verify syntax * check-yaml - attempts to load all yaml files to verify syntax * end-of-file-fixer - ensures files end in a newline and only a newline * mixed-line-ending - replaces mixed line endings with LF * trailing-whitespace - trims trailing whitespace * python-check-blanket-noqa - enforces that noqa annotations always occur with specific codes Changes made to comply with new hooks: * Remove trailing whitespaces * Remove some useless noqa annotations * Specify errors for noqa annotations * Add missing newlines at end of files See: python-discord/organisation#138
MarkKoz
added
area: dependencies
MarkKoz
force-pushed
the
feat/deps/o138/pre-commit-hooks
branch
from
020141f to
242c6d1
Compare
This also means flake8 will output to stdout in CI now. It didn't before because it output to an XML format for publishing. Pre-commit creates its own environment in which it installs hooks. To speed up runs, the pipeline will cache this for use with future jobs. The cache will update if .pre-commit-config.yaml changes. The flake8 pre-commit hook invokes flake8 via `pipenv run flake8`. It's normally useful to use pipenv here cause it ensures flake8 is invoked within the context of the venv. However, in CI, there is no venv - dependencies are installed directly to the system site-packages. `pipenv run` does not work in such case because it tries to create a new venv if one doesn't exist (it doesn't consider the system interpreter to be a venv). This workaround (okay, it's a hack) creates an executable shell script which replaces the original pipenv binary. The shell script simply ignores the first argument (i.e. ignores `run` in `pipenv run`) and executes the rest of the arguments as a command. It essentially makes `pipenv run flake8` equivalent to just having ran `flake8`. When pre-commit executes pipenv, the aforementioned script is what will run.
MarkKoz
force-pushed
the
feat/deps/o138/pre-commit-hooks
branch
from
242c6d1 to
21dcf7a
Compare
SebastiaanZ
left a comment
SebastiaanZ
left a comment
There was a problem hiding this comment.
Looks good. There's one diff I don't quite get, the last line of CONTRIBUTING.MD, but that's just because I don't see a difference; I'm probably overlooking something and there's nothing wrong with the changed version.
It's removed an empty line from the end of file. |
That's what I suspected; the GitHub diff just isn't showing that. It shows that difference for other removed lines at the end of files, but for this one it shows up as if the line the diff is referencing has been removed and inserted unchanged. This: vs: Guess it's a GH diff glitch. |
|
Yep. I had to download the raw files to confirm it just in case; github wasn't showing the correct diff for it as you said. |
lemonsaurus
left a comment
lemonsaurus
left a comment
There was a problem hiding this comment.
Yep, this all looks good. I guess I don't fully understand the newline at end fixer, I thought it would ensure newlines at the end of any kind of file but it seems this is not the case. Can anyone shed some light on this?
Anyway, it looks good enough to merge, and I'm sure the fixer is doing what it should be doing.
| # See e.g. https://stackoverflow.com/a/38588882/4464570 | ||
| *.png binary | ||
| *.whl binary | ||
|
|
There was a problem hiding this comment.
Is this a result of running the end-of-file-fixer?
If so, I don't really understand why this file doesn't get a newline at the end of the file. I thought every kind of file was supposed to have one but maybe there's more to it?
There was a problem hiding this comment.
This was an extra newline at the end of the file. It had two final newlines. Now it has one. Some things, like GitHub, git, and neovim, aren't showing the final newline as a separate line, while others, like PyCharm, are.
| max-width: none; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Another special rule for css files?
I guess there must be files then that should not have newlines to the end of the file.
eivl
left a comment
eivl
left a comment
There was a problem hiding this comment.
Looks fine, I do not really understand the azure-pipelines configuration well enough to comment anything other than what it looks like.
5 participants
Relevant Issues
python-discord/organisation#138
Description
New hooks were added for pre-commit and they will run in CI too. The
pipenv run lintscript will now run all the new hooks, including flake8. As a side effect of running in CI, flake8's output will now be shown in stdout in addition to the XML that's published.Hooks added
A couple of these hooks automatically apply fixes. However, they still report failure and leave any changes they make uncommitted. Therefore, the user has to commit the automatic fixes.
check-merge-conflict- Check for files that contain merge conflict strings.check-toml- Attempts to load all toml files to verify syntax.check-yaml- Attempts to load all yaml files to verify syntax.end-of-file-fixer- Makes sure files end in a newline and only a newline.mixed-line-ending- Replaces mixed line endings with LF.trailing-whitespace- Trims trailing whitespace.python-check-blanket-noqa- Enforce thatnoqaannotations always occur with specific codesReasoning
Additional Details
The pre-commit venv is cached. There should only be a cache miss if the
.pre-commit-config.yamlfile changes or if the location of the Python interpreter changes (more realistically, if the Python version changes). Maybe Azure wipes the cache sometimes too, but in that case pre-commit will simply re-create the environment.