mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anton Chernov <mecher...@gmail.com>
Subject Re: Squash/Merge PRs
Date Thu, 12 Jul 2018 12:19:41 GMT
Unfortunately there has been significant push back for small iterative PR's
and as a result they have grown substantially involving multiple
contributors, multiple commits, sometimes multiple branches to be worked on.

I fully agree and support all points that Jin raised:

1) Most contributions should be broken down into smallest possible PR's.
2) If a PR is small enough a single person can complete it.
3) If a majority of PR is done by someone, and there's some minor issue
he/she needs help with it could be done in a follow up PR by anybody,
including the reviewer.

Best regards,
Anton

чт, 12 июл. 2018 г. в 10:11, Hao Jin <hjjn.amzn@gmail.com>:

> +1 for Marco's proposal on using the co-author field. IMHO the usage of
> co-author field should not be that often, consider:
> 1) If a PR is so big that it needs multiple people to contribute a
> substantial part of it, why can't that PR be broken down into smaller PRs
> each submitted by single one of them?
> 2) If a PR is small enough (for example, < 300 lines), why can't a single
> person complete it?
> 3) If a majority of PR is done by someone, and there's some minor issue
> he/she needs help with(a small bug, incomplete doc), why can't that be done
> through code reviews?
> From the above 3 situations we can see that collaborations on individual
> PRs should not be quite often, but I do agree that it could still be
> necessary when someone lacks the related expertise/knowledge on some
> necessary part of that PR given that the required knowledge cannot be
> picked up in a short period of time.
>
> I do agree that keeping the commit histories of PRs clean is very important
> as it could be confusing when reviewing PRs, but that really depends on
> personal preferences (For my case, I usually do "git commit --amend" on
> trivial changes and get a new commit for bigger changes such as a
> checkpoint of my whole PR). With growing the community and attracting more
> contributors as a high priority for our project, I would prefer that we do
> not put even more burden on the contributors by asking them to manage and
> squash the commits themselves just for the not-so-often cases of having
> multiple contributors on a single PR.
> Best regards,
> Hao
>
> On Thu, Jul 12, 2018 at 12:35 AM, Marco de Abreu <
> marco.g.abreu@googlemail.com.invalid> wrote:
>
> > Hi Naveen,
> >
> > I'm in favour of the squashing, considering the number of commits in some
> > PRs and especially because of some people making commit messages a la
> "fix"
> > "fix" "fix" all the time. Additionally, it gets hard (not impossible,
> just
> > more inconvenient) to determine the atomic states of master - aka, which
> > commits are separate from each other. You should consider that
> intermediary
> > commits are unstable (fail CI) and thus it could be very hard to bisect
> > failures in future - and the commit history gets cluttered.
> >
> > As alternative, I'd like to suggest the co-author field for these cases.
> > Further documentation is available at
> > https://blog.github.com/2018-01-29-commit-together-with-co-authors/.
> >
> > I definitely agree with the second part. We should all lead by example
> and
> > maintain a high quality by keeping our commit messages clean and
> > meaningful. When I receive an email notification that a new commit has
> been
> > added and it only contains "fix" as title, it's not that helpful and also
> > it's hard to track the development of a PR overtime. E.g., why has
> > something been changed? Was there maybe a bug that we didn't cover with
> > tests but the author just hacked something to get it to work but the
> > problem still lays somewhere? We won't know that way and it makes it
> harder
> > for us to review.
> >
> > Best regards,
> > Marco
> >
> >
> > Naveen Swamy <mnnaveen@gmail.com> schrieb am Do., 12. Juli 2018, 10:09:
> >
> > > Hi All,
> > >
> > > I am seeing that maintainers merge PRs into the repo, they are
> squashing
> > > the commits in the PR, which I understand and agree is to keep a sane
> > > commit history, however this is causing problem when there are multiple
> > > contributors involved on a PR(by contributing to a fork of the repo)
> this
> > > effectively removes credit for multiple contributors involved and shows
> > all
> > > code as authored by the contributor who created the PR.
> > >
> > > Can I request maintainers to not squash PRs if there are multiple
> > > contributors involved on the PR.
> > >
> > > Also on the same note, I request contributors(regardless of multiple
> > > contributors or not) to keep a clean commit history by squashing the
> > > commits and not push all your WIP commits to the PR. this will help us
> > keep
> > > our commit history clean and meaningful.
> > >
> > > Let me know your thoughts/better approach or If I have misunderstood
> how
> > > this works.
> > >
> > > Thanks, Naveen
> > >
> >
>

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