Skip to content

Repo - MINOR - Format the repository and run the pre-commit check on every pull request - #356

Merged
logan-nc merged 16 commits into
developfrom
test/enable-formatter-ci-gate
Nov 2, 2026
Merged

logan-nc merged 16 commits into
developfrom
test/enable-formatter-ci-gate

Conversation

@matt-pharr

@matt-pharr matt-pharr commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Release note

  • Audience: developers
  • Numerical impact: none (harness @ 1883bee): all 27 cases unchanged; gal_resistive_pe is N/A by design
  • Migration: run pre-commit run --all-files before pushing. The pre-commit check now runs on every pull request into develop and main. To bring an open branch in line, see the notes below.

The repository is formatted with the pinned pre-commit hooks, and the pre-commit workflow runs on every pull request, so formatting churn stops showing up in PR diffs. Closes #515.

What changed

Commit Change
(Pharr's commits) .github/workflows/format.yaml adds the pull_request trigger for develop and main.
9e57323f9 The output of pre-commit run --all-files on develop at 961484b92, and nothing else. JuliaFormatter 1.0.62 is the version .pre-commit-config.yaml and the workflow pin. 99 files change: Julia formatting, trailing whitespace, and end-of-file fixes (including seven coil-geometry .dat files and Project.toml). Every TOML convention check already passed.
ee76864c6 The workflow comment now says the check is enabled together with the sweep (it used to say it was expected to fail until the sweep). The check also runs on pushes to develop, because two pull requests that are each clean can merge into unformatted code.
f7fe63ce8 docs/src/set_up.md pins JuliaFormatter 1.0.62. The unpinned Pkg.add("JuliaFormatter") it used to suggest installs 2.x, which formats differently and would fail this check on a contributor's first PR. CLAUDE.md's formatting section now describes the pinned version and the CI check.
1883beeb0 .git-blame-ignore-revs lists the sweep commit, so GitHub's blame skips it. Locally, run git config blame.ignoreRevsFile .git-blame-ignore-revs.

How to review the sweep

The sweep commit is reproducible: regenerating it from 961484b92 in a fresh worktree reproduces it byte for byte. To check:

git checkout 961484b92        # develop the sweep was made from
julia -e 'using Pkg; Pkg.add(PackageSpec(name="JuliaFormatter", version="1.0.62"))'
pre-commit run --all-files    # run twice: the end-of-file and formatter hooks interact once
git diff 9e57323f9 -- . ':!.github'   # expect no output
  • The coil .dat files: they only lose trailing spaces, and read_coil_dat splits on whitespace, so they read the same.
  • This PR's own pre-commit job: it runs here with the new trigger and must pass.

Bringing an open branch in line

  1. Merge develop as of just before the sweep: git merge 6cca8d77c.
  2. Run pre-commit run --all-files and commit the result.
  3. Run git merge -X ours <this PR's merge commit>. Where both sides reformatted the same lines, this keeps the branch's own formatted lines.
  4. Run pre-commit run --all-files again; it must come back clean.
  5. Check that git diff against develop shows only the branch's real changes.

Done this way, the branch's diff carries no formatting churn. PRs #625, #626 and #627 have already been brought in line this way.

Regression report

Harness on feynman: develop 961484b92 vs this branch at 1883beeb0 (the later commits touch only .github/ and docs)
tj_n1                          25 unchanged
diiid_slayer_n1_toroidal       5 unchanged
gal_resistive_diiid_galinner   11 unchanged
ggj_ray_q500i                  4 unchanged
solovev_kinetic_nuzero         14 unchanged
diiid_error_field              10 unchanged
diiid_slayer_n1                22 unchanged
diiid_truncation_n1            12 unchanged
gal_resistive_pe               8 missing/N/A
tj_kinetic_calculated          20 unchanged
diiid_slayer_n1_coupled        7 unchanged
gal_resistive_diiid            11 unchanged
solovev_n1                     22 unchanged
ggj_reference                  4 unchanged
tj_kinetic_ntv                 7 unchanged
solovev_multi_n                15 unchanged
diiid_n1                       53 unchanged
diiid_slayer_n1_chi_perp_e     22 unchanged
diiid_n1_riccati               17 unchanged
lar_ggj_tearing_n1             3 unchanged
diiid_multi_n                  19 unchanged
diiid_slayer_n1_winding        22 unchanged
efit_fixedbdy_separatrix       5 unchanged
tj_kinetic_multiion            6 unchanged
diiid_kinetic_multi_n          8 unchanged
diiid_n1_riccati_precompiled   17 unchanged
lar_reversed_shear_n1          22 unchanged

gal_resistive_pe reads N/A on both refs by design: its perturbed-equilibrium stage waits on #635.

Notes for reviewers

  • Authorship: the trigger is Pharr's, and logan-nc added the sweep. That's why the reviewer should be someone other than either of them.
  • Merge method: use a merge commit, not squash, so the hash in .git-blame-ignore-revs stays valid. If it's squashed, .git-blame-ignore-revs needs a follow-up with the new hash.
  • Timing: merge promptly once approved. Any later merge into develop makes the sweep stale and means re-running it, which takes minutes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF

@logan-nc

Copy link
Copy Markdown
Collaborator

@matt-pharr assign someone or mark as draft

@matt-pharr matt-pharr added this to the GPEC v2.0.0 milestone Oct 6, 2026

@logan-nc logan-nc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Measured what this gate would see today, since nobody had posted the size of the sweep it waits for. JuliaFormatter.format(".") at v1.0.62 (the rev pinned in .pre-commit-config.yaml, so the gate and a local run agree) on develop db90e82 rewrites 88 files, +1212/−1064, spread over every module; the hygiene hooks additionally flag trailing whitespace in seven text files (.github/workflows/test.yaml and six benchmarks/*.jl). So the gate as written would fail every PR until that sweep lands, as the comment in the diff says. Sequencing suggestion: the sweep collides with #569 (36 files), #574 (39), #581 (35), #551 (31) and #593 (17), so the cheap order is those five first, then the one-off sweep, then this PR the same day. The diff itself is fine; one nit: format.yaml has no concurrency block while #593 adds cancel-in-progress to test.yaml, so a rapid push sequence will queue formatter runs.

@matt-pharr

Copy link
Copy Markdown
Collaborator Author

@logan-nc I say this can go in after #602, which can end with a formatter commit.

@logan-nc

Copy link
Copy Markdown
Collaborator

closes #515

@logan-nc logan-nc added refactor Behavior-preserving restructuring test Tests only labels Oct 30, 2026
@logan-nc

logan-nc commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

@matt-pharr Adding the one-off pre-commit run --all-files sweep to this PR, so the gate lands already green and closes #515 together with it. The sweep is a single mechanical commit plus a .git-blame-ignore-revs entry. Your trigger change stays as is. Say if you'd rather keep this PR to the trigger only, and I'll move the sweep out.

logan-nc and others added 3 commits November 1, 2026 19:49
Output of `pre-commit run --all-files` with JuliaFormatter 1.0.62 (the version
.pre-commit-config.yaml and the format workflow pin) on develop at 961484b:
Julia formatting, trailing whitespace and end-of-file fixes. Nothing else.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF
@logan-nc logan-nc changed the title TEST - enable formatting - Update pre-commit trigger for pull requests in workflow Repo - MINOR - Format the repository and run the pre-commit check on every pull request Nov 2, 2026
@logan-nc
logan-nc marked this pull request as ready for review November 2, 2026 00:54
@logan-nc
logan-nc self-requested a review November 2, 2026 00:54
@github-actions github-actions Bot added the minor Not worth reporting label Nov 2, 2026
@github-actions github-actions Bot removed test Tests only refactor Behavior-preserving restructuring labels Nov 2, 2026
logan-nc and others added 2 commits November 1, 2026 20:01
…n it on pushes to develop

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF
…be the pre-commit CI check

An unpinned Pkg.add installs JuliaFormatter 2.x, which formats differently and
would fail the check on every contributor's first PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdRsGGg4YRG6E9fgLGbpdF

@logan-nc logan-nc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review of the sweep and the CI change. No changes requested. Everything I found is already pushed to this branch.

The sweep (9e57323f9) is reproducible. Re-running pre-commit run --all-files (JuliaFormatter 1.0.62) on develop 961484b92 in a fresh worktree reproduces it byte for byte. The only difference is this PR's workflow file, which isn't on develop.

It's layout only.

  • The only change inside string literals is five one-line docstrings turned into """ blocks, with the same text.
  • No string macros or quoted code were touched, and no [a -b]-style matrix literal changed meaning.
  • The seven coil-geometry .dat files only lose trailing spaces, and read_coil_dat splits on whitespace.
  • Project.toml only loses two trailing blank lines.

CI: all green except the harness stamp. pre-commit, Tests, Documentation (so the reformatted docstrings render) and both Julia versions pass.

Fixed on this branch:

  • docs/src/set_up.md told contributors to run Pkg.add("JuliaFormatter") without a version. That installs 2.x, which would fail this check on everyone's first PR. It now pins 1.0.62.
  • CLAUDE.md said the formatter version "is not currently pinned"; it now describes the pinned version and the check.
  • The workflow comment said the check was "expected to fail until the sweep lands"; corrected.
  • Added a push trigger on develop, so two clean PRs that merge into unformatted code are still caught.

Pending: the harness run, develop vs this branch (feynman job 137273, ETA about 23:30 EST). It should be bit-identical, and I'll add the stamp to the release note when it finishes. The "Title and release note" check is failing only because that stamp is missing.

When merging: add it to the queue, which uses merge commits, so the .git-blame-ignore-revs hash stays valid. Merge it before #625, #626 and #627; they already contain this sweep.

@logan-nc
logan-nc added this pull request to the merge queue Nov 2, 2026
Merged via the queue into develop with commit 5503af4 Nov 2, 2026
19 of 21 checks passed
@logan-nc
logan-nc deleted the test/enable-formatter-ci-gate branch November 2, 2026 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Not worth reporting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Repo - we need to run the Julia Formatter on the entire repo and ensure consistency going forward

2 participants