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

ENH: Initial configuration to introduce pre-commit #5013

Merged

Conversation

hjmjohnson
Copy link
Member

Update SetupForDevelopment to setup pre-commit

Using a temporary repo to hold Kitware hooks with a configuration for
pre-commit. Replaces the system to clone the hook repo/branch into the
.git/hooks directory, which just using pre-commit.

This PR mirrors work from SimpleITK performed by
Bradley Lowekamp [email protected]

The pre-commit python virtual environment can be manually
loaded with: source .git/hooks/venv/bin/activate

which would allow using the pre-commit command prior to
doing an actual commit.

i.e.

source .git/hooks/venv/bin/activate
pre-commit run -a

PR Checklist

Related to: #4238
Related to: #4565
Related to: #4244

@hjmjohnson hjmjohnson added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Dec 4, 2024
@hjmjohnson hjmjohnson added this to the ITK 6.0 Beta 1 milestone Dec 4, 2024
@hjmjohnson hjmjohnson requested a review from blowekamp December 4, 2024 16:19
@hjmjohnson hjmjohnson self-assigned this Dec 4, 2024
@hjmjohnson hjmjohnson force-pushed the test-pre-commit-hooks branch from 12ddb0c to 1729522 Compare December 4, 2024 16:27
@blowekamp
Copy link
Member

I made an upstream pull request for Kitware's GitSetup to support precommit to avoid the code duplication:
https://gitlab.kitware.com/utils/gitsetup/-/issues/18#note_1499602
But it was rejected due to it being more python specific and not as generic and efficient as the shell system infrastructure.

Also in #4651 I started to use clang-format into pre-commit.

Sometimes with big rebases and commit the large number of recommit checks is slower, so perhaps there can be too many checks. However I have found the precommit file easier to maintain.

@hjmjohnson
Copy link
Member Author

I made an upstream pull request for Kitware's GitSetup to support precommit to avoid the code duplication: https://gitlab.kitware.com/utils/gitsetup/-/issues/18#note_1499602 But it was rejected due to it being more python specific and not as generic and efficient as the shell system infrastructure.

Also in #4651 I started to use clang-format into pre-commit.

Sometimes with big rebases and commit the large number of recommit checks is slower, so perhaps there can be too many checks. However I have found the precommit file easier to maintain.

Pre-commit is tremendously easier to maintain. I don't think the performance concerns are that disruptive.

@hjmjohnson hjmjohnson changed the title WIP: Initial configuration to introduce pre-commit ENH: Initial configuration to introduce pre-commit Dec 4, 2024
@hjmjohnson hjmjohnson marked this pull request as ready for review December 4, 2024 23:20
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Thanks for doing this Hans.

Have not tried locally.

A few comments:

@blowekamp
Copy link
Member

I made an upstream pull request for Kitware's GitSetup to support precommit to avoid the code duplication: https://gitlab.kitware.com/utils/gitsetup/-/issues/18#note_1499602 But it was rejected due to it being more python specific and not as generic and efficient as the shell system infrastructure.
Also in #4651 I started to use clang-format into pre-commit.
Sometimes with big rebases and commit the large number of recommit checks is slower, so perhaps there can be too many checks. However I have found the precommit file easier to maintain.

Pre-commit is tremendously easier to maintain. I don't think the performance concerns are that disruptive.

Yes, I agree it's likely worth the switch. Just need to consider ways to reduce code duplication etc.

@hjmjohnson hjmjohnson mentioned this pull request Dec 5, 2024
2 tasks
@hjmjohnson hjmjohnson force-pushed the test-pre-commit-hooks branch from 2beb005 to 00d1248 Compare December 5, 2024 02:21
@github-actions github-actions bot added the type:Enhancement Improvement of existing methods or implementation label Dec 5, 2024
@hjmjohnson hjmjohnson force-pushed the test-pre-commit-hooks branch from 13ddc1c to a117d80 Compare December 5, 2024 02:59
@github-actions github-actions bot added area:Examples Demonstration of the use of classes area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Segmentation Issues affecting the Segmentation module area:Numerics Issues affecting the Numerics module area:Documentation Issues affecting the Documentation module labels Dec 5, 2024
Utilities/Maintenance/VCL_ModernizeNaming.py: has a shebang but is not marked executable!
pre-commit hook id: end-of-file-fixer
pre-commit hook id: trailing-whitespace
Remove the cmake based custom download and running of
clang-format with shell scripting.

Prefer the more robust and easier to maintain
pre-commit clang-format instrumentation.
@hjmjohnson hjmjohnson force-pushed the test-pre-commit-hooks branch from 4c32da8 to 8b99c00 Compare December 6, 2024 13:59
@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:Examples Demonstration of the use of classes area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Segmentation Issues affecting the Segmentation module area:Numerics Issues affecting the Numerics module area:Documentation Issues affecting the Documentation module labels Dec 6, 2024
@hjmjohnson hjmjohnson requested a review from blowekamp December 6, 2024 14:21
@hjmjohnson
Copy link
Member Author

@dzenanz @blowekamp This should be ready for final reviews now. I have it working on my Mac, and older testing that I think is the same, worked on windows and linux.

@@ -6,7 +6,6 @@ include(${_ITKModuleMacros_DIR}/ITKModuleAPI.cmake)
include(${_ITKModuleMacros_DIR}/ITKModuleDoxygen.cmake)
include(${_ITKModuleMacros_DIR}/ITKModuleHeaderTest.cmake)
include(${_ITKModuleMacros_DIR}/ITKModuleKWStyleTest.cmake)
include(${_ITKModuleMacros_DIR}/ITKModuleClangFormat.cmake)
Copy link
Member

Choose a reason for hiding this comment

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

@N-Dekker Ahh, with this commit, we will not be required to "know" about existence of clang-format. Except for use in IDEs, such as Visual Studio 😄

@hjmjohnson hjmjohnson merged commit 66b88db into InsightSoftwareConsortium:master Dec 6, 2024
17 checks passed
@hjmjohnson hjmjohnson deleted the test-pre-commit-hooks branch December 6, 2024 19:31
@blowekamp
Copy link
Member

I just encountered an issue with updating ThirdParty libraries and the precommit setup:

++ git commit --no-gpg-sign -n '--author=OpenJPEG Upstream <[email protected]>' '--date=2024-12-09 17:31:23 +0100' -F -
No .pre-commit-config.yaml file was found
- To temporarily silence this, run `PRE_COMMIT_ALLOW_NO_CONFIG=1 git ...`
- To permanently silence this, install pre-commit with the --allow-missing-config option
- To uninstall pre-commit run `pre-commit uninstall`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module area:Documentation Issues affecting the Documentation module area:Examples Demonstration of the use of classes area:Filtering Issues affecting the Filtering module area:Numerics Issues affecting the Numerics module area:Python wrapping Python bindings for a class area:Segmentation Issues affecting the Segmentation module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants