geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dan Smith <dsm...@pivotal.io>
Subject Re: what is the best way to update a geode pull request
Date Fri, 31 May 2019 20:31:30 GMT
+1 to pushing PR changes as separate commits. Also +1 to creating PRs with
multiple commits where it makes sense.

-Dan

On Fri, May 31, 2019 at 1:28 PM Owen Nichols <onichols@pivotal.io> wrote:

> Personally, I do not force-push to my PRs once any review comments have
> been accumulated, for the reasons you mention.
>
> Not sure if some people just force-push out of habit, or if the
> requirement for initial commit to be squashed creates some fear.
>
> I would go a step further and suggest that there is no reason to even
> require the initial PR to be a single, squashed commit.  If you have made a
> small fix and also done some refactoring or cleanup, as a reviewer I would
> much rather you submit that PR with multiple distinct commits, so I don’t
> have to comb through a gigantic diff trying to spot what is the real change
> and what is just renames.
>
> GitHub already very nicely presents the combined delta by default, as if
> it was a single squashed commit, so to me there is only negative value in
> encouraging/requiring pre-squashing.
>
> > On May 31, 2019, at 1:20 PM, Darrel Schneider <dschneider@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.
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message