Skip to content

Add pre-commit hook for autoformatting#2233

Open
cyrus- wants to merge 2 commits into
devfrom
pre-commit-hook
Open

Add pre-commit hook for autoformatting#2233
cyrus- wants to merge 2 commits into
devfrom
pre-commit-hook

Conversation

@cyrus-
Copy link
Copy Markdown
Member

@cyrus- cyrus- commented Apr 27, 2026

Summary

  • Adds a shared pre-commit hook (under scripts/git-hooks/) that runs dune fmt --auto-promote and re-stages any originally-staged files the formatter touched, so commits never contain unformatted code.
  • Files that weren't staged are deliberately left alone — unrelated unstaged WIP won't be swept into the commit.
  • make install-hooks sets core.hooksPath to scripts/git-hooks. It's idempotent and is now a dependency of deps, dev, and dev-student, so anyone running setup or a normal build picks up the hook automatically (no manual opt-in needed).
  • git commit --no-verify still bypasses the hook for cases where it's necessary.

Test plan

  • make install-hooks (first run installs, second run is silent — verified)
  • Commit with no formatting changes — hook runs, commit goes through
  • Stage a misformatted file with no other unstaged changes — formatted version lands in commit
  • Stage a misformatted file while having unrelated unstaged edits in another file — only the staged file is in the commit (formatted); unrelated WIP stays unstaged
  • git commit --no-verify — bypasses the hook
  • Reviewer: try make deps once on an existing clone to confirm hooks auto-install

🤖 Generated with Claude Code

cyrus- added 2 commits April 27, 2026 17:13
Stored under scripts/git-hooks/ and installed via core.hooksPath. Hook
runs `dune fmt --auto-promote` and re-stages any of the originally-staged
files that the formatter touched, so commits never contain unformatted
code. Files that weren't staged are left alone, even if the formatter
modified them — so unrelated unstaged WIP doesn't get swept into the
commit.

Wired install-hooks into deps/dev so existing developers pick up the
hook on their next make invocation.
Move install-hooks off make dev / dev-student — it only needs to run
during initial setup. Add a Git Hooks section to CONTRIBUTING.md and
a one-line pointer in README.md so contributors know what make deps
is wiring up.
@cyrus- cyrus- requested a review from 7h3kk1d April 27, 2026 16:20

if [ -n "$staged" ]; then
while IFS= read -r f; do
[ -e "$f" ] && git add -- "$f"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is going to behave interestingly in the case where someone partially stages a file. Resulting in the entire file being committed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having thought about this more my personal preference is probably to just abort the commit if there's reformatting. So I would probably stash the unstaged components run formatting (abort if there's changes) then commit then unstash.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alternatively we can have github reformat on the server

@7h3kk1d
Copy link
Copy Markdown
Contributor

7h3kk1d commented Apr 28, 2026

@Negabinary can you try this with vs code + windows. Not sure how it'll work there.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 50.39%. Comparing base (64e7def) to head (e4396c2).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2233      +/-   ##
==========================================
- Coverage   50.42%   50.39%   -0.04%     
==========================================
  Files         296      296              
  Lines       39452    39452              
==========================================
- Hits        19894    19880      -14     
- Misses      19558    19572      +14     

see 16 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

2 participants