+1 to updating the PR template. I've noticed that few people actually follow it and sometimes they just remove it altogether. +1 to pushing PR changes as separate commits. This makes PR review easier.
Sometimes it's helpful to me as a reviewer for the initial PR to be split into multiple commits as well. Also, I like Robert's suggestion of merge instead of rebase when the PR is out-of-date with develop. - Aaron On Fri, May 31, 2019 at 2:50 PM Jacob Barrett <jbarr...@pivotal.io> wrote: > > > > On May 31, 2019, at 2:40 PM, Udo Kohlmeyer <u...@apache.org> wrote: > > > > If we are concerned about the single line that can break the product, > then our testing has failed us on so many levels, that there is no hope. > > Sorry, I used a hyperbolic statement about looking for 1 line out of 1000. > The point was “formatting” or “cleanup” style commits are better left > separate because looking for the real change in that sea of change is hard. > > > But looking forward to see how long one can sustain the "factor -> > commit -> make changes required to fulfill JIRA -> commit -> manual merge"… > > It’s only a problem if you are cleaning up lots a code. Not a bad problem > to have and the future looks brighter each time. > > > Also, who reviews the refactor, because even that can introduce > unintentional bugs... same effort as single commit. In single commit, if > the refactor has made code cleaner, clearer and simpler, maybe 1 commit is > easier to follow. > > I think there is a distinction between a refactor and cleanup. Consider > the time we decide to reformat all the code, that was a cleanup. Now as we > are going through the code and IJ reports every other line as a static > analyzer warning, fixing that is a cleanup. All these cleanups have been > reviews like any other PR. Th point being made was that they are done in a > way that allows the reviewer to review the clean and the change > independently. > > A refactor would would be a complete reorganization of code and should > have the tests, reviews, etc. that go with it. > > Regardless, all are reviewed. > > -Jake > >
