Add optional pre-commit hook for code formatting#5178
Open
jamescrosswell wants to merge 17 commits intomainfrom
Open
Add optional pre-commit hook for code formatting#5178jamescrosswell wants to merge 17 commits intomainfrom
jamescrosswell wants to merge 17 commits intomainfrom
Conversation
Adds a pre-commit git hook that verifies code formatting before allowing commits. This helps catch formatting issues early and reduces CI failures. Implementation: - `.githooks/pre-commit`: Runs `dotnet format --verify-no-changes` - `scripts/setup-hooks.sh`: One-time setup script to configure git hooks - Updated CONTRIBUTING.md with setup instructions The hook is optional but recommended. If formatting issues are detected, the commit is prevented and the developer is prompted to run `dotnet format` manually. The hook uses the same dotnet format command as CI, ensuring consistency. Addresses #4980 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5178 +/- ##
==========================================
+ Coverage 74.06% 74.08% +0.02%
==========================================
Files 501 506 +5
Lines 18113 18247 +134
Branches 3521 3564 +43
==========================================
+ Hits 13415 13519 +104
- Misses 3838 3858 +20
- Partials 860 870 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Previously the hook piped dotnet format output through grep to check for "formatted" string. This ignored the actual exit code, which meant: - If dotnet format failed for unrelated reasons (missing SDK, etc) but didn't output "formatted", the hook would pass incorrectly - The implementation diverged from how dotnet format is intended to work Now uses --verify-no-changes' exit code directly (non-zero when formatting is needed), which is more reliable and matches the tool's design. Addresses Warden feedback in review comment. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ModuleInitializer is legitimately used in test projects for test setup (VerifyHttp initialization, EF provider registration, etc.). Test projects are effectively application code, not libraries, so this usage is appropriate. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changed the hook to replicate CI's approach: - Stash unstaged changes with --keep-index - Run dotnet format (no --verify-no-changes, matches CI exactly) - Check git diff to detect formatting changes - Restore stashed changes This avoids false failures from analyzer warnings (IDE1006, CA2255, etc.) and only fails on actual formatting changes that would fail CI. The stash approach handles mixed staged/unstaged changes cleanly - we only check formatting on what's being committed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace the manual stash-pop block with a `trap ... EXIT` handler so unstaged changes are restored even when `dotnet format` fails and `set -e` causes early termination. Also replace the silent `2>/dev/null || true` swallow with an explicit failure message so developers are notified when a stash-pop conflict leaves their changes in the stash. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When git stash pop fails due to a conflict, git leaves conflict markers in the affected files and keeps the stash entry — running stash pop again won't help. Update the warning to tell the developer to resolve the conflict markers first, then drop the stash. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The stash/unstash dance was error-prone (data loss on format failure, silent conflicts on pop). Just run dotnet format and block the commit if it made changes — the developer handles their own working tree. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds `./dev.cs setup-hooks` as a discoverable way to configure git to use the repo's pre-commit hooks from .githooks/. Calling git config directly in C# means it works on Windows too, replacing the Unix-only scripts/setup-hooks.sh approach. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Superseded by `./dev.cs setup-hooks` which does the same thing cross-platform with no duplicate logic to keep in sync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use `--include` with the list of staged .cs files so the format run only touches what's actually being committed. Unstaged WIP is left completely alone. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
grep exits 1 on no matches, which set -e would treat as a failure. Adding || true to the assignment makes this robust regardless of pipefail settings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Symmetric counterpart to setup-hooks — runs git config --unset core.hooksPath to restore default git hooks behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the space-separated string + unquoted expansion with a bash array, passing each staged file as its own --include argument. This correctly handles paths containing spaces or shell metacharacters. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dotnet format operates on the working tree, not the index, so partially staged files would have their unstaged hunks modified too. Skip with a warning instead — the check still runs on clean commits (the common case). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace removed scripts/setup-hooks.sh with ./dev.cs setup-hooks - Correct hook behaviour: auto-fixes formatting rather than verify-only - Document the unstaged changes skip behaviour - Add ./dev.cs remove-hooks opt-out instructions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Redirecting all output to /dev/null meant build errors or analyzer failures silently blocked the commit with no explanation. Capture output and print it only when dotnet format exits non-zero. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d1f2687. Configure here.
Extensionless hook scripts fell through to * text=auto, risking CRLF line endings on Windows checkouts which break the bash shebang. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Adds an optional pre-commit git hook that auto-fixes code formatting before allowing commits. This helps catch formatting issues early and reduces CI failures.
Implementation
New files:
.githooks/pre-commit: Hook script that runsdotnet formatagainst staged.csfiles onlyUpdated files:
dev.cs: Addssetup-hooksandremove-hookscommands to opt in/outBehavior:
./dev.cs setup-hooksonce to enable the hookdotnet formatruns against staged.csfiles only and auto-fixes any formatting issuesgit add -u) and commits againgit commit --no-verifyif needed./dev.cs remove-hooksto opt outDesign decisions:
git add -uand recommit rather than manually runningdotnet format--includescoped to staged.csfiles so only what's being committed gets formattedAddresses
Fixes #4980
Next Steps
This PR makes the hook optional. After battle testing for a bit, we could:
🤖 Generated with Claude Code