3 ms·
> If I get a PR to review I want the set of commits to be clean and represent logical changes. No “oops” commits. +100. Though every time this comes up on HN y
by omegalulw 5y ago
> If I get a PR to review I want the set of commits to be clean and represent logical changes. No “oops” commits.
+100. Though every time this comes up on HN you would be surprised by how much some people just vehemently disagree.
I will also add another requirement - all unit tests should pass at each commit in the PR. That way, you can use the rebase strategy to merge and you would still be able to bisect.
- alkonaut 5y agoPassing unit tests on each commit is only a realistic goal if developers can run all tests locally in less than (say) an hour. If the test suite is ten hours and you have ten commits on a branch then it quickly becomes silly to bog down your build servers (or cloud bills) with building and testing the intermediate commits over 90h. Using your own machine to do it (blocking it from doing other work) is obviously not a good idea either.
- omegalulw 5y ago> If the test suite is ten hours and you have ten commits on a branch then it quickly becomes silly to bog down your build servers That's a fair point, but I would say that if you encourage your devs to make each commit logical, meaningful and self contained, this is worth the extra cost. Also, you don't need to run ALL unit tests - just the ones affected by commit you are testing. This of course requires a build system with good dependency analysis.