+1 for doing whatever facilitates reviewing and is best for the PR (which may vary by PR or even reviewers) -1 to disallowing or even strongly discouraging force pushing in a PR (go ahead and merge or rebase as each person prefers but don't force that preference on others)
I prefer to separate cleanup/refactor commits from behavior change commits but I accept that it's on a per-case or per-author basis. I care more about cleaning up code and adding unit tests than making the reviewing easier. On Fri, May 31, 2019 at 5:32 PM Aaron Lindsey <alind...@pivotal.io> wrote: > +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 > > > > >
