Add simple pre-commit hook #1065
Conversation
.github/workflows/unit_tests.yml
Outdated
| @@ -12,17 +12,27 @@ jobs: | |||
| runs-on: ubuntu-22.04 | |||
| steps: | |||
| - uses: actions/checkout@v2 | |||
|
|
|||
There was a problem hiding this comment.
There are things we can improve in this file, like updating the version of the Actions we use. I noticed that #1036 touches that, so I didn't do anything in order to avoid conflicts.
| - name: Set up Python 3.10 | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: "3.10" | ||
| - name: Install dependencies | ||
|
|
||
| # Note: we could try and cache the pre-commit environment |
There was a problem hiding this comment.
I tried doing it, but my first attempt didn't work. I still had the
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
but the entire pre-commit step only takes 12 seconds at the moment. So it seems okay to leave it as is for now.
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install pre-commit | ||
| pre-commit run --all-files |
There was a problem hiding this comment.
This does modify the files, but we don't commit them. The important thing is that the command's exit code is set to 1 if it made changes, which makes the workflow fail.
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v3.2.0 | ||
| hooks: | ||
| - id: trailing-whitespace |
There was a problem hiding this comment.
As I mentioned in #237, this trailing-whitespace is the one that I find important.
This file is just the output of pre-commit sample-config, and the default seemed reasonable. I'm okay removing the others if anyone insists.
| @@ -160,6 +160,21 @@ No data should be lost since it is just re-generateable build files. | |||
| [pythondevmode]: https://docs.python.org/3/library/devmode.html | |||
| [pudb]: https://pypi.org/project/pudb/ | |||
|
|
|||
| ## Setting up a development environment | |||
There was a problem hiding this comment.
This could eventually move into its own file within https://github.com/getting-things-gnome/gtg/tree/master/docs/contributors. I think it's OK to have it here for now.
I will create a task for modernizing https://github.com/getting-things-gnome/gtg/blob/master/docs/contributors/coding%20best%20practices.md, which still mentions Python 2 and tox. It's a good place to also mention the automated code quality checks that we're adding.
nekohayo
added
the
maintainability
SqAtx
force-pushed
the
add_pre_commit
branch
2 times, most recently
from
d8a0051 to
ce8184c
Compare
SqAtx
force-pushed
the
add_pre_commit
branch
2 times, most recently
from
cb92fb3 to
74acdad
Compare
|
@diegogangl @nekohayo what do you guys think of this one? |
|
Hey, thanks for working on this! LGTM, though I'd split it at least the whitespace changes from the yaml changes in two different commits. Also, looks like I caused some conflict here (hopefully just whitespace stuff) |
|
Yep that' s a good point. I'll rebase this with just my changes. After you merge it, you can just run pre-commit and make a new commit that will fix the build. I also just noticed that https://github.com/getting-things-gnome/gtg/blob/master/Makefile is a thing, and we probably want to eventually move pep8/pyflakes out of it and into pre-commit. I think between meson and pre-commit, that makefile will be pretty much redundant? |
Relates to getting-things-gnome#237 This adds a simple configuration for pre-commit and runs it on GHA. It doesn't do very much; mostly removing trailing whitespaces. My next step would be to add pylint or pyflakes to this, and tell it to ignore every warning that GTG currently breaks. This would: * prevent us from breaking any more * allow us to fix the warnings one at a time, when we can I tested the GHA on a fork. This commit doesn't contain the changes that fix the pre-commit check, so GHA will fail. To fix it, we'll just need to run `pre-commit run --all-files` and commit the result.
SqAtx
changed the title
|
I've never used it but I don't mind trying it! It seems pretty young but has quite a few developers behind it. And it integrates with pre-commit, so why not. |
Relates to #237
This adds a simple configuration for pre-commit and runs it on GHA. It doesn't do very much; mostly removing trailing whitespaces.
My next step would be to add pylint or pyflakes to this, and tell it to ignore every warning that GTG currently breaks. This would:
I tested the GHA on a fork.
This commit doesn't contain the changes that fix the pre-commit check, so GHA will fail. To fix it, we'll just need to run
pre-commit run --all-filesand commit the result.