4 ms·
> It means that the code review process is more important that the code writing process. If that were true, the optimal solution is PRs with individual commit
by jldugger 11y ago
> It means that the code review process is more important that the code writing process.
If that were true, the optimal solution is PRs with individual commits that all pass testing. I find it much easier to review a series of small changes for logical correctness than mashing them together into a single PR. Github recently added this as a feature, so I'm not in a completely invisible minority there.
And then, when the review is over, having discreet commits makes git bisecting down to the commit that broke the system more granular.
- phasmantistes 11y agoAbsolutely. If someone formulates their PR such that every commit in the chain is small, easily reviewable, and passes all tests, that's fantastic! That makes reviewing code, searching history, and bisecting all easier. Unfortunately, that's not the 90% case that I see. Most of the time a multi-commit PR contains N-1 commits of incremental development and one final one that fixes all the tests and typos and removes debugging print statements. Neither the project nor the author benefit from having those intermediate commits integrated verbatim.
- jarfil 11y agoIf people generate N-1 commits with a final one to clean things up, maybe people should learn about git stash and making some WIP branches, then squashing commits themselves or better yet, keeping their own history clean, instead of submitting PRs full of crap. I know, it might be too much to ask of people... oh well.
- hinkley 11y agoThe Mikado method is sadly underemphasized. Just because you smash away at code for hours doesn't mean that's how your commit history should look. You can revise history in ways that are beneficial instead of destructive.
- cortesoft 11y agoWhat is the cost of just doing it at the last merge step? Makes it easier, doesn't it?
- chris_wot 11y agoYeah, but dreadful if you try to bisect a problem.
- rjayatilleka 11y agoYeah, and that's why you squash all the intermediate commits first. If all your commits on master are useful and meaningful individually, bisect works great.
- forrestthewoods 11y ago> Neither the project nor the author benefit from having those intermediate commits integrated verbatim. Sure they benefit. You can see the thought process that went into the commit. If there's something that seems weird or out of place you can see how it evolved into existence. That can be exceptionally useful.
- stingraycharles 11y agoI agree with what you're saying, good code review and good code writing aren't mutually exclusive. However, the truth of the matter is that code review is a far more clearly defined moment in a workflow than writing good code (which is more of a habit than an actual action). As such, it is far more efficient for a team to accept reality and squash after PRs (which is now first possible), instead of relying and keep correcting all your coworkers when they don't properly squash commits before pushing.
- voidlogic 11y ago>If that were true, the optimal solution is PRs with individual commits that all pass testing. I find it much easier to review a series of small changes for logical correctness than mashing them together into a single PR. This is too myopic and explains how you can end up with good code, but bad architecture. A good review ensures both. I like my PRs to be about high level goals, and I want them made of lots of commits I can review. The commits themselves can be the result of re-basing (which is fine within feature branches) and maybe not truly chronological.