Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ENH: Add pre-commit GitHub Action #5239

Merged

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Feb 11, 2025

Run our pre-commit checks.

@thewtex thewtex requested review from N-Dekker and Copilot February 11, 2025 16:09

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@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 area:Documentation Issues affecting the Documentation module type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module labels Feb 11, 2025
@thewtex thewtex force-pushed the pre-commit-gh-action branch from e859a3d to f85e6a8 Compare February 11, 2025 16:38
@github-actions github-actions bot added area:Core Issues affecting the Core module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module labels Feb 11, 2025
@thewtex thewtex force-pushed the pre-commit-gh-action branch from 3b1825a to 9e06ec9 Compare February 11, 2025 16:51
Run our pre-commit checks.

The pre-commit GHA will install via `python -m pip`.

Configure hooks.SetupForDevelopment for pre-commit GHA.
Set to a high number to avoid local developer check.
Also mock user.email, user.name for local hooks.
Applied via:

 pixi run pre-commit-run
@thewtex thewtex force-pushed the pre-commit-gh-action branch from 9e06ec9 to 8915e0e Compare February 11, 2025 16:55
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Maybe we should hold off the last 2 commits for now?

@@ -248,7 +248,6 @@ class ConnectedImageNeighborhoodShape
{
return (includeCenterPixel ? 1 : 0) + CalculateNumberOfConnectedNeighbors(maximumCityblockDistance);
}

Copy link
Member

Choose a reason for hiding this comment

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

Some of these spaces removals will counteract changes from #3805.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a hard time following the issue and resolution in #3805.

These were necessary to get the CI green.

Last available on mirrors-clang-format.

The procedure described in InsightSoftwareConsortium@98f0ffa
was followed to ensure no changes were required to the .clang-format
configuration files.
clang-format 19.1.7 and pre-commit 4.1.0.
@thewtex thewtex force-pushed the pre-commit-gh-action branch from 8915e0e to 2659871 Compare February 12, 2025 16:16
@github-actions github-actions bot added the area:Examples Demonstration of the use of classes label Feb 12, 2025
@thewtex
Copy link
Member Author

thewtex commented Feb 12, 2025

Maybe we should hold off the last 2 commits for now?

These are necessary to get the CI check green.

@thewtex thewtex requested a review from jhlegarreta February 12, 2025 16:21
@thewtex thewtex requested a review from hjmjohnson February 12, 2025 16:21
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.

Re: #5239 (comment). I've noticed that, but it looks like the changes in PR #3805 did not have the desired effect, or else, ther were more warnings that were raised after that:
https://open.cdash.org/viewBuildError.php?type=1&buildid=10204557

😔 We will need to revisit the unbalanced warnings issue. Approving.

@thewtex thewtex merged commit f3a62a7 into InsightSoftwareConsortium:master Feb 13, 2025
16 checks passed
@thewtex thewtex deleted the pre-commit-gh-action branch February 13, 2025 01:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
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:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module 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.

5 participants