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: Add pre-commit configuration file to run clang-format #4651

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

blowekamp
Copy link
Member

The "pre-commit" be installed congenitally or with the Utilities/GitSetup/setup-precommit installation script.

This version of clang-tidy can be run on all files by pre-commit run -a.

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation labels May 8, 2024
@github-actions github-actions bot removed the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label May 9, 2024
Copy link
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

@blowekamp I love this idea!!! I have found that pre-commit is much more reliable than our custom built tools. It makes it much easier to keep the codebase clean than does our current system.

After this initial commit is in place, I'd be happy to propose a set of other changes that will make maintenance a lot easier for new developers.

@blowekamp
Copy link
Member Author

@blowekamp I love this idea!!! I have found that pre-commit is much more reliable than our custom built tools. It makes it much easier to keep the codebase clean than does our current system.

After this initial commit is in place, I'd be happy to propose a set of other changes that will make maintenance a lot easier for new developers.

You may be interested in what I did in SimpleITK:
https://github.com/SimpleITK/SimpleITK/blob/master/.pre-commit-config.yaml

@thewtex thewtex added this to the ITK 6.0 Beta 1 milestone May 13, 2024
cd "${BASH_SOURCE%/*}" &&

# check if python executable exists and is at least MIN_PYTHON_VERSION
if ! command -v python3 &> /dev/null; then
Copy link
Member

@thewtex thewtex May 13, 2024

Choose a reason for hiding this comment

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

On Windows Git Bash, command -v python3 is going succeed even if Python is not installed or Python is installed but python.exe is not in the PATH. python3 is a non-functional stub that suggests installing Python via the Windows store.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a suggestion to what would work here?

Copy link
Member

Choose a reason for hiding this comment

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

I looked into using uv briefly, but it complained about not being able to find python -- it will require more digging or investigation into other approaches.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@thewtex thewtex May 20, 2024

Choose a reason for hiding this comment

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

I have heard that uv has plans to distribute python, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two things here: 1) is python3 installed and working and 2) If it's not can the ITK configuration automagically install and use a version of python.

Perhaps we can only address #1 in this initial PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, only 1) can be addressed first, but the commit hook should be optional when python3 is not available (hook setup should or execution should not fail).

Copy link
Contributor

Choose a reason for hiding this comment

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

automagically installing python seems frought, there are so many OSes and ways to install python...

Copy link
Member

Choose a reason for hiding this comment

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

It is not a system install, just a local binary, similar to the commit hooks themselves.

@dzenanz dzenanz mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Enhancement Improvement of existing methods or implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants