Skip to content
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

Fixes #660 - Add pre-commit hooks #663

Conversation

jiridanek
Copy link
Contributor

@jiridanek jiridanek commented Aug 5, 2022

Here's how this fails in CI, https://github.com/skupperproject/skupper-router/runs/7690658958?check_suite_focus=true#step:4:11

You can skip hooks by either not installing them (see below) or by doing git commit --no-verify.

Here's how this fails locally, when hooks are installed

% git commit                              
git clang-format.........................................................Failed
- hook id: git_clang-format
- exit code: 1
- files were modified by this hook

ERROR: you need to run git clang-format on your commit

For local usage, it is necessary to install the pre-commit hook first, by running

python3 -m pip install -r requirements-dev.txt
pre-commit install

@jiridanek jiridanek self-assigned this Aug 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #663 (48aa062) into main (5a68b6f) will not change coverage.
The diff coverage is n/a.

❗ Current head 48aa062 differs from pull request most recent head fde18d5. Consider uploading reports for the commit fde18d5 to get more accurate results

@@           Coverage Diff           @@
##             main     #663   +/-   ##
=======================================
  Coverage   25.90%   25.90%           
=======================================
  Files         128      128           
  Lines       31195    31195           
  Branches     4973     4973           
=======================================
  Hits         8081     8081           
  Misses      22059    22059           
  Partials     1055     1055           
Flag Coverage Δ
unittests 25.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jiridanek jiridanek force-pushed the jd_2022_08_05_clang_format_precommit_changed_lines branch 6 times, most recently from 59c6921 to b3f84c2 Compare August 5, 2022 11:53
@jiridanek jiridanek changed the title Fixes # 660 Add pre-commit hooks Fixes #660 - Add pre-commit hooks Aug 5, 2022
@jiridanek jiridanek marked this pull request as ready for review August 5, 2022 12:00
@jiridanek jiridanek force-pushed the jd_2022_08_05_clang_format_precommit_changed_lines branch from b3f84c2 to a7cfe79 Compare August 5, 2022 12:09
@jiridanek jiridanek linked an issue Aug 5, 2022 that may be closed by this pull request
Copy link
Contributor

@kgiusti kgiusti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work the way I'd expect - maybe I don't understand how to use it.

I've installed pre-commit and git-clang-format via dnf.

I've modified message.c to introduce format errors. I then commit message.c.... and it commits without error.

I had expected the commit to fail due to the format errors but it did not.

Ideally we want to flag the error in a way that it's impossible to ignore... and commit upstream.

Also: can cmake check that git-clang-format and per-commit are installed and fail if not? Treat them as required for the build?

scripts/git-clang-format Show resolved Hide resolved
scripts/git-clang-format Show resolved Hide resolved
scripts/git_clang-format_hook.sh Outdated Show resolved Hide resolved
@jiridanek
Copy link
Contributor Author

I've installed pre-commit and git-clang-format via dnf.

Did you do pre-commit install in the skupper-router directory?

@kgiusti
Copy link
Contributor

kgiusti commented Aug 5, 2022

I've installed pre-commit and git-clang-format via dnf.

Did you do pre-commit install in the skupper-router directory?

That's it: I missed that part.

Now when I attempt to commit a bad format I get:

]$ git commit -am "debug: do not merge"
[INFO] Initializing environment for local:clang-format.
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
git clang-format.........................................................Failed

  • hook id: git_clang-format
  • exit code: 1
  • files were modified by this hook

ERROR: you need to run git clang-format on your commit

The nice thing is that it already fixed the file locally. All I needed was to re-run the commit and it succeeded.

Copy link
Contributor

@kgiusti kgiusti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well, thanks.

@jiridanek jiridanek force-pushed the jd_2022_08_05_clang_format_precommit_changed_lines branch from a7cfe79 to 399cfa5 Compare August 5, 2022 15:26
@jiridanek jiridanek merged commit efc9bbd into skupperproject:main Aug 5, 2022
@jiridanek jiridanek deleted the jd_2022_08_05_clang_format_precommit_changed_lines branch August 5, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add git precommit with clang-format
3 participants