style: add pre-commit hooks and editorconfig #385
Conversation
Without wanting to overload the pre-commit hooks, this adds a few checks on the commits to ensure validity of files. More time-consuming checks for linting and formatting are done pre-push to avoid impacting the developer experience during local development. These are covered by `prettier` (for markdown, yaml, json, ...), `black` for Python and `ruff` for Python linting. To help standardise formatting in the future, an `.editorconfig` file has also been added to the repository. By virtue of using `pre-commit`, only modified files are checked which ensures older files are incrementally updated. Signed-off-by: JP-Ellis <josh@jpellis.me>
JP-Ellis
added
smartbear-supported
|
I haven't seen that tool before, but am familiar with the likes of husky etc. How does the (I realise you haven't introduced this check) |
|
From what I can tell As for This sets up the various Pre-commit also has a GitHub Action (which unfortunately is in maintenance mode) and its own pre-commit.ci which is free for open source repositories. This is quite useful for one-off contributors as the CI workflow can automatically fix basic formatting issues, thereby reducing the barriers to contributors while still ensuring a consistent style and catching simple lint issues early (and hopefully avoiding bugs later on). This PR does not enforce any additional checks in the CI workflow, as the existing codebase is generally not very compliant. Whether I incrementally get the repository into compliance, or do a huge 'big bang' commit, I'll decide later. |
|
Ah, OK that makes sense. I was wondering how you'd enforce that to run locally but it seems like it is an opt-in thing - perhaps we should update the CONTRIBUTING.md file? Perhaps you could enable that check for PR builds only? That way, new contributors would still have that check applied without having to install it? |
|
The whole As for adding checks in the CI workflow, happy to try it out. I don't know whether it will work as I believe the CI workflow checks are executed against the whole codebase, as opposed to just the changed files. Let me test it :) |
|
I like the idea of pre-commit hooks, because the amount of times I pushed changes up only to have a linter complain about something that could be auto-corrected, is a bit annoying, but I don't think everyone uses them. I would be great to have the same experience in CI, so that the linting errors would be caught. it would be nicer if like you say you can just do it against the diff of the changed files, rather than the whole code, on a PR - if that is a possibility that would be a great option I feel like there was discussion over in the pact-js camp with regards to pre-commit hooks and their viability, but will have to do some sleuthing to find it |
|
Ok, so I have tested the pre-commit GitHub Action and unfortunately it runs the checks against the entire codebase. So it is not a viable solution to implement. The hooks when run locally are still aware of what files have changed (though admittedly, if a file is touched at all, the entire file must be compliant), but won't require the entire codebase to be compliant. This will allow for code to incrementally become compliant. At this stage, I would recommend we just implement pre-commit hooks as purely opt-in. At a later point in time, it might be worth looking at getting the entire codebase into compliance and having some discussions as to what we want that to look like. |
JP-Ellis
force-pushed
the
style/editorconfig-precommit
branch
from
01ba089
to
e75470d
Compare
|
👋🏾 So I pulled down this branch, checked into a new branch, edited some python files, added the files, gave it a garbage commit message. Should it be doing something before the commit is registered. What do do I need to pre install to set this up? |
|
The hooks do depend on the developer having By default, pact-python/.pre-commit-config.yaml Lines 1 to 4 in e75470d As for the garbage commit messages, I have not used |
|
I'm okay with this at the moment because its opt in, and there is no issue if you don't have it installed, as I've found, and changes will be linted in ci. running hatch lint across the codespace in scary at the moment. Curious as to why its so non compliant, but was okay with flake8. I assume various reasons, no need to go into them all, will see them, as they begin to get addressed 👍🏾 |
|
I know you are planning on overhauling the developing docs but would just add a pre reqs section here https://github.com/pact-foundation/pact-python/blob/master/CONTRIBUTING.md with hatch required, pre-commit (useful, optional) |
|
Cool, this is really nice. Installing pre-commit 🕙16:40:33 ❯ brew install pre-commit
Running `brew update --auto-update`...
==> homebrew/core is old and unneeded, untapping to save space...
Untapping homebrew/core...
Untapped 3 commands and 6697 formulae (7,065 files, 473.7MB).
==> Auto-updated Homebrew!
Updated 3 taps (cirruslabs/cli, homebrew/core and homebrew/cask).
==> New Formulae
biome colmap dovi_tool modsecurity mtbl orcania postgresql@16 uffizzi web-ext yder
==> New Casks
akuity ava chainner cloudnet meld-studio mutedeck playdate-mirror reqable rustrover twelite-stage wetype
Warning: Calling the `appcast` stanza is deprecated! Use the `livecheck` stanza instead.
Please report this issue to the adoptopenjdk/openjdk tap (not Homebrew/brew or Homebrew/homebrew-core), or even better, submit a PR to fix it:
/opt/homebrew/Library/Taps/adoptopenjdk/homebrew-openjdk/Casks/adoptopenjdk8.rb:9
You have 37 outdated formulae installed.
Warning: Calling the `appcast` stanza is deprecated! Use the `livecheck` stanza instead.
Please report this issue to the adoptopenjdk/openjdk tap (not Homebrew/brew or Homebrew/homebrew-core), or even better, submit a PR to fix it:
/opt/homebrew/Library/Taps/adoptopenjdk/homebrew-openjdk/Casks/adoptopenjdk8.rb:9
Warning: Calling the `appcast` stanza is deprecated! Use the `livecheck` stanza instead.
Please report this issue to the adoptopenjdk/openjdk tap (not Homebrew/brew or Homebrew/homebrew-core), or even better, submit a PR to fix it:
/opt/homebrew/Library/Taps/adoptopenjdk/homebrew-openjdk/Casks/adoptopenjdk8.rb:9
Warning: Calling the `appcast` stanza is deprecated! Use the `livecheck` stanza instead.
Please report this issue to the adoptopenjdk/openjdk tap (not Homebrew/brew or Homebrew/homebrew-core), or even better, submit a PR to fix it:
/opt/homebrew/Library/Taps/adoptopenjdk/homebrew-openjdk/Casks/adoptopenjdk8.rb:9
==> Downloading https://ghcr.io/v2/homebrew/core/pre-commit/manifests/3.4.0
#=#=- # # #=O#- # # ######################################################################################################################################################################################################################################################### 100.0%
==> Fetching dependencies for pre-commit: pyyaml and virtualenv
==> Downloading https://ghcr.io/v2/homebrew/core/pyyaml/manifests/6.0.1
######################################################################################################################################################################################################################################################### 100.0%
==> Fetching pyyaml
==> Downloading https://ghcr.io/v2/homebrew/core/pyyaml/blobs/sha256:7e52df0812b2d3714c1d1504cbd07597aea578b1646e35ce2275fc484dd50957
#=#=- # # #=O#- # # -#O=- # # # ######################################################################################################################################################################################################################################################### 100.0%
==> Downloading https://ghcr.io/v2/homebrew/core/virtualenv/manifests/20.24.5
######################################################################################################################################################################################################################################################### 100.0%
==> Fetching virtualenv
==> Downloading https://ghcr.io/v2/homebrew/core/virtualenv/blobs/sha256:0ff65ef186573fd2d156f24557f7131a14daa7ede7b58afb2c71ed51a4c71b84
#=#=- # # #=O#- # # ######################################################################################################################################################################################################################################################### 100.0%
==> Fetching pre-commit
==> Downloading https://ghcr.io/v2/homebrew/core/pre-commit/blobs/sha256:f805e7aef476961d05d917f846fdc193d026beee9888bb164de3bcac34d46654
#=#=- # # #=O#- # # ######################################################################################################################################################################################################################################################### 100.0%
==> Installing dependencies for pre-commit: pyyaml and virtualenv
==> Installing pre-commit dependency: pyyaml
==> Downloading https://ghcr.io/v2/homebrew/core/pyyaml/manifests/6.0.1
Already downloaded: /Users/yousaf.nabi/Library/Caches/Homebrew/downloads/5ca7699e2fc3568e2c66065d0042209c7df23ae1514ef3ec4aae83f03654de94--pyyaml-6.0.1.bottle_manifest.json
==> Pouring pyyaml--6.0.1.arm64_ventura.bottle.tar.gz
🍺 /opt/homebrew/Cellar/pyyaml/6.0.1: 59 files, 1016.4KB
==> Installing pre-commit dependency: virtualenv
==> Downloading https://ghcr.io/v2/homebrew/core/virtualenv/manifests/20.24.5
Already downloaded: /Users/yousaf.nabi/Library/Caches/Homebrew/downloads/4164c7a30daed8110b695b6b0dfecb3ee9fec0bf30d57cb9932c97b8457e1733--virtualenv-20.24.5.bottle_manifest.json
==> Pouring virtualenv--20.24.5.arm64_ventura.bottle.tar.gz
🍺 /opt/homebrew/Cellar/virtualenv/20.24.5: 180 files, 5MB
==> Installing pre-commit
==> Pouring pre-commit--3.4.0.arm64_ventura.bottle.tar.gz
🍺 /opt/homebrew/Cellar/pre-commit/3.4.0: 142 files, 799.6KB
==> Running `brew cleanup pre-commit`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).Lets remove a last line white space from a file and try to commit (we haven't actually setup pre-commit on our repo) wu-oh - that is allowed and going to fail in CI 🕙16:41:20 ❯ gst
On branch style/editorconfig-precommit
Your branch is up to date with 'origin/style/editorconfig-precommit'.
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: pact/http_proxy.py
no changes added to commit (use "git add" and/or "git commit -a")
pact-python on style/editorconfig-precommit@origin:style/editorconfig-precommit [$!] via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙16:42:52 ❯ ga .
pact-python on style/editorconfig-precommit@origin:style/editorconfig-precommit [$+] via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙16:42:55 ❯ gcmsg 'whoops'
[style/editorconfig-precommit b854a06] whoops
1 file changed, 1 insertion(+), 1 deletion(-)Lets setup pre-commit on our repo 🕙16:43:37 ❯ pre-commit install
pre-commit installed at .git/hooks/commit-msg
pre-commit installed at .git/hooks/pre-commit
pre-commit installed at .git/hooks/pre-pushLets try and commit again - lovely it auto-fixed it, and leaves us to perform our commit again, after adding the file to our staged changes 🕙16:44:28 [🔴 ERROR] ❯ gst
On branch style/editorconfig-precommit
Your branch is ahead of 'origin/style/editorconfig-precommit' by 1 commit.
(use "git push" to publish your local commits)
Changes to be committed:
(use "git restore --staged <file>..." to unstage)
modified: .pre-commit-config.yaml
modified: pact/http_proxy.py
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: pact/http_proxy.py
pact-python on style/editorconfig-precommit@origin:style/editorconfig-precommit [$!+⇡] via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙16:44:40 ❯ gcmsg 'whoops'
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /Users/yousaf.nabi/.cache/pre-commit/patch1695138290-32160.
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
check for broken symlinks............................(no files to check)Skipped
detect destroyed symlinks................................................Passed
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook
Fixing pact/http_proxy.py
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
check toml...........................................(no files to check)Skipped
check xml............................................(no files to check)Skipped
check yaml...............................................................Passed
check json5..........................................(no files to check)Skipped
[WARNING] Stashed changes conflicted with hook auto-fixes... Rolling back fixes...
[INFO] Restored changes from /Users/yousaf.nabi/.cache/pre-commit/patch1695138290-32160.Lets add our fix performed by pre-commit, and try again pact-python on style/editorconfig-precommit@origin:style/editorconfig-precommit [$!+⇡] via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙16:44:51 [🔴 ERROR] ❯ ga .
pact-python on style/editorconfig-precommit@origin:style/editorconfig-precommit [$+⇡] via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2)
🕙16:45:00 ❯ gcmsg 'whoops'
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
check for broken symlinks............................(no files to check)Skipped
detect destroyed symlinks................................................Passed
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
check toml...........................................(no files to check)Skipped
check xml............................................(no files to check)Skipped
check yaml...............................................................Passed
check json5..........................................(no files to check)Skipped
[INFO] Installing environment for https://github.com/commitizen-tools/commitizen.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
check for case conflicts.................................................Passed
check for broken symlinks............................(no files to check)Skipped
detect destroyed symlinks................................................Passed
mixed line ending........................................................Passed
check toml...........................................(no files to check)Skipped
check xml............................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
check json5..........................................(no files to check)Skipped
commitizen check.........................................................Failed
- hook id: commitizen
- exit code: 14
commit validation: failed!
please enter a commit message in the commitizen format.
commit "": "whoops"
pattern: (?s)(build|ci|docs|feat|fix|perf|refactor|style|test|chore|revert|bump)(\(\S+\))?!?:( [^\n\r]+)((\n\n.*)|(\s*))?$Nice, catches our commit messages, we would only be told to update it, as part of the PR review anyway, and awesome to get a list of acceptable values and the regex used 😽 pact-python on style/editorconfig-precommit@origin:style/editorconfig-precommit [$+⇡] via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2) took 17s
🕙16:45:19 [🔴 ERROR] ❯ gcmsg 'test: whoops'
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
check for broken symlinks............................(no files to check)Skipped
detect destroyed symlinks................................................Passed
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
check toml...........................................(no files to check)Skipped
check xml............................................(no files to check)Skipped
check yaml...............................................................Passed
check json5..........................................(no files to check)Skipped
check for case conflicts.................................................Passed
check for broken symlinks............................(no files to check)Skipped
detect destroyed symlinks................................................Passed
mixed line ending........................................................Passed
check toml...........................................(no files to check)Skipped
check xml............................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
check json5..........................................(no files to check)Skipped
commitizen check.........................................................Passed
[style/editorconfig-precommit 6ed814c] test: whoops
2 files changed, 3 insertions(+), 4 deletions(-)
pact-python on style/editorconfig-precommit@origin:style/editorconfig-precommit [$⇡] via 🐳 desktop-linux via 🐍 v3.11.4 on ☁️ (eu-west-2) took 2s Lovely! |
I think there's two aspects here:
Thanks 🥳 I will make sure to have the docs updated as part of #386 :) (along with the |
Without wanting to overload the pre-commit hooks, this adds a few checks on the commits to ensure validity of files.
More time-consuming checks for linting and formatting are done pre-push to avoid impacting the developer experience during local development. These are covered by
prettier(for markdown, yaml, json, ...),blackfor Python andrufffor Python linting.To help standardise formatting in the future, an
.editorconfigfile has also been added to the repository.By virtue of using
pre-commit, only modified files are checked which ensures older files are incrementally updated.