+1 to updating the checklist. About single vs multiple commits: I think either is fine as long as it is deliberate and makes it easier to understand what someone is doing. If there is a large refactor that enables a small change, it is often easier to understand when these changes are in separate commits. At that point one might consider splitting this even out into separate PRs, but sometimes seeing that the change is following the refactor provides valuable context. It also reduces review latency. As so often: Think about what you are doing and why and do what makes sense rather than blindly follow a simplistic rule.
On Fri, May 31, 2019 at 1:44 PM Jacob Barrett <jbarr...@pivotal.io> wrote: > Would that be PR of a the PR template. Might that cause a black hole to > form? > > I’m in favor of updating this wall of text you smack your face on for each > PR. Let’s pair it down to discourage people (myself strongly included) from > ignoring or deleting it. > > -jake > > > > On May 31, 2019, at 1:33 PM, Anthony Baker <aba...@pivotal.io> wrote: > > > > Let’s update the checklist to match the outcome of this thread: > > > https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md > < > https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md > > > > > > Anthony > > > > > >> On May 31, 2019, at 1:31 PM, Helena Bales <hba...@pivotal.io> wrote: > >> > >> +1. I would guess that it is the checklist as part of the PR that is > >> confusing people. > >> > >> The other reason that history gets rewritten is when force pushing > after a > >> rebase. While fast-forwarding is necessary on occasion, this can be > >> accomplished without rewriting history by using a merge. > >> > >> As part of our document on making PRs, we should include instructions on > >> how to handle the situation where fast-forwarding is necessary, > explicitly > >> discourage the use of merges and force-pushes once a PR has been opened, > >> and some guidelines regarding the appropriate number of commits when > the PR > >> is initially opened. Once we have these guidelines, it would be helpful > to > >> link to them from the PR checklist that we currently have, and rework > the > >> checklist so that it is in line with our desired process. > >> > >> On Fri, May 31, 2019 at 1:20 PM Darrel Schneider <dschnei...@pivotal.io > > > >> wrote: > >> > >>> Something I have noticed is that often when I have requested changes be > >>> made to a pull request is that the changes are force pushed ask a > single > >>> commit to the pr. I would actually prefer that the changes show up as > a new > >>> commit on the pr instead of everything being rebased into one commit. > That > >>> makes the history of the pr easier to follow and make it easy to see > what > >>> has changed since the previous review. What do others think? Have we > done > >>> something that makes contributors think the pull request has to be > single > >>> commit? I know the initial pull request is supposed to be but from > then on > >>> I'd prefer that we wait to squash when we merge it to develop. > >>> > > >
