I think we are on the same page here. There is only one thing that is different from what we are doing in PRs.
> Before you create a PR, squash all applicable commits to make it more > readable for reviewers > I would suggest not squashing everything before creating the PR. I mean, do the following when coding: - disable your auto formatter (in Eclipse you can just disable the save actions) if you are going to change a class that might need formatting - do the changes - create a commit for the main changes of the PR - enable the auto formatter and formatting the change files - create a commit for formatting and cleanup changes - then create the PR Separating formatting/cleanups commits makes it easier for reviewers to evaluate changes. On Tue, May 1, 2018 at 3:53 PM, Tutkowski, Mike <mike.tutkow...@netapp.com> wrote: > Hi everyone, > > We had a good conversation going here. Maybe we can continue it, get some > level of reasonable consensus, and implement it (if, in fact, the consensus > is a change from what we currently have). > > My suggested approach is the following: > > Before you create a PR, squash all applicable commits to make it more > readable for reviewers. Once reviews start coming in and you start making > changes, push new commits on top of the prior ones (do not squash at this > point). This will make it easier for reviewers to confirm that you and they > are on the same page with regards to what was changed. When you need to > draw in changes from the base branch, rebase your commits on top of it. > When the PR is given a LGTM by 2+ reviewers and passed the necessary > regression tests, it should be squashed and then merged. I see the > evolution of commits during the life of the PR as a temporary sandbox of > history that is no longer required once the PR has been completed. > > I think that process should cover the vast majority of our PRs. > > There are usually some exceptions to the rule, however. When this happens, > discuss your situation with the reviewers and bring any concerns to the > mailing list before deviating from the standard process. > > Thoughts? > Mike > > On 1/15/18, 1:47 PM, "Rene Moser" <m...@renemoser.net> wrote: > > > > On 01/15/2018 09:06 PM, Rafael Weingärtner wrote: > > Daan, > > > > Now that master is open for merges again, can we get a feedback > here? It > > might be interesting to find a consensus and a standardize way of > working > > for everybody before we start merging things in master again … > > +1 to allow merge commits on master branch to keep history of a series > of patches when they help to understand the change. > > René > > > > > -- Rafael Weingärtner
