Conversation
The list is empty; a reformatting commit adds its own hash here so that git blame skips it.
|
@seanm If we are going to diverge from the main nifti_c lib, I would propose making as many changes as necessary in one commit. I prefer to use clang format. I have had better luck with integration into my editor workflows. Additionally, we could keep in sync with ITK and reduce the overall maintenance headaches. |
|
I guess I thought at least making the indentation uniform would make it easier to work on the codebase, because currently it's a pain since touching different parts requires using different indentation, which my editor does not like. I'd also like to use clang-format eventually. I could give it another try, but even after using |
|
I think what @hjmjohnson is saying is we should merge all the "formatting cleanup" into a single bulk PR so we can ignore it for blame purposes (this would also be a place to setup a clang-format hook and github action to block merging any format fails. |
Ignoring things in blame doesn't require one big commit though, you can put as many SHAs as you want in a |
Closed PR comment#26 does what was asked for here, so this can probably be closed. @hjmjohnson's point above — clang-format rather than uncrustify, to stay in sync with ITK — is what it implements: ITK's @seanm, on your comment that "even after using This branch is 18 months stale and |
|
The three config files here are worth having. The uncrustify run that comes with them is what makes this hard to land, and I think the two should be separated. Recommendation: keep What the diff actually consists ofMeasured against the merge-base, counting only files that already existed:
Why the reformat half is expensive right nowIt rewrites 8 source files in Landing the reformat means every one of them has to be rebased through a whole-file reindentation, where git's rename and content detection is least reliable and a silent mis-resolution is most likely. The backlog is mostly confirmed bug fixes; paying that cost to land a change with no functional effect inverts the priority. It is also The config files stand on their own
If the reformat is still wanted afterward, the version that is cheap to review is one that runs uncrustify over the whole tree in a single commit, once the bug backlog has drained, with that commit's SHA added to |
0cec098 to
64974a1
Compare
|
Commit messages cleaned up in place (prefixes added, Recommendation, unchanged: land the three config commits, defer the reformat. Why no rebase
Measured on the branch as it stands: 2,486 added lines, of which 2,426 change only whitespace ( The split, and why the case is stronger nowThe four commits divide cleanly:
The first three conflict with nothing, are useful the moment they land, and make the fourth reproducible by anyone at any later date. The count of other open PRs touching the 8 files the reformat rewrites has gone from 15 to 21 since the earlier review: #21, #22, #24, #38, #39, #42, #43, #44, #47, #48, #49, #50, #52, #53, #55, #56, #58, #61, #67, #72, #74. Landing the reformat invalidates every one of them at once. Landing it after they merge costs nothing, because Two small things to fix whenever this is picked up
Commit messagesRewritten in place with Nothing was stripped: no commit here carried a tool trailer, a session URL, or a bare cross-reference. No build or test runNot run, and not meaningful for this branch in its current state: the reformat commit cannot be compiled against current |
No description provided.