mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pedro Larroy <pedro.larroy.li...@gmail.com>
Subject Re: [DISCUSSION] Adding labels to PRs
Date Mon, 15 Jan 2018 10:55:37 GMT
+1

Agree with Bhavin, Marco and Sheng. I would also like to point out
good commit practices such as, keeping each individual commit small
and on-topic, meaning if that you are changing whitespace and a one
liner fix, it's better practice to separate those commits. Or having
two separate commits for unrelated changes. I see that often we are
squashing these kinds of commits, this makes it more difficult to
review changes and then bisect when you need to find problems. I see
many times them squashed, and I wonder if there's a general rule that
we are following.

On Sun, Jan 14, 2018 at 9:55 PM, Marco de Abreu
<marco.g.abreu@googlemail.com> wrote:
> +1, we have quite a lot of commits and PR titles which don't state the
> content clearly.
>
> On another note, we should strongly encourage PRs serving only one purpose.
> I have seen quite a decent amount of PRs which introduce multiple unrelated
> changes, while only the main change is getting advertised in the title as
> well as in the description.
>
> -Marco
>
> On Sun, Jan 14, 2018 at 9:32 PM, Bhavin Thaker <bhavinthaker@gmail.com>
> wrote:
>
>> +1 to having committers act as role models for descriptive PR title and
>> descriptions.
>>
>> I am sorry to say this explicitly because I found a few committers to have
>> no descriptions in their PRs with PR titles that seemed vague to me as a
>> contributor of Apache MXNet.
>>
>> Bhavin Thaker.
>>
>> On Sun, Jan 14, 2018 at 12:27 PM, Sheng Zha <zhasheng@apache.org> wrote:
>>
>> > +1. Also, I think committers should act as role models in this regard,
>> and
>> > ensure that our own code changes have sufficient details in the PR.
>> >
>> > Since I proposed the PR template, I also want to re-state that any
>> > suggestions to improving the PR template are welcome. By being open about
>> > it, we can build consensus through discussions, and then act accordingly.
>> > Thanks.
>> >
>> > -sz
>> >
>> > On 2018-01-14 12:21, Bhavin Thaker <bhavinthaker@gmail.com> wrote:
>> > > While this email thread had 3 proposals earlier and most comments were
>> on
>> > > Jira Vs Github issues, I want to bring attention to and request all
>> > > committers to NOT merge a code change unless the PR title and
>> description
>> > > has sufficient details as required in the PR template.
>> > >
>> > > Both the PR title and the description are useful for Apache MXNet users
>> > in
>> > > understanding the code changes in a release from the Release Notes,
>> > > especially when a user has a problem and wants to know if a particular
>> > > release may fix the problem or not.
>> > >
>> > > This email is based on feedback from the Release retrospective
>> checklist
>> > > documented here (prepared after the MXNet 1.0 Release):
>> > > https://cwiki.apache.org/confluence/display/MXNET/
>> > Release+Retrospective+for+Apache+MXNet+%28incubating%29+1.0.0
>> > >
>> > > Any comments or suggestion on the above Release retrospective are most
>> > > welcome.
>> > >
>> > > Bhavin Thaker.
>> > >
>> > > On Mon, Nov 13, 2017 at 3:03 PM, Stephen Bull <sb7145@gmail.com>
>> wrote:
>> > >
>> > > > As a complete noob here and someone without a JIRA account, it seems
>> I
>> > can
>> > > > at least view JIRA issues (so a plus for regular users when it comes
>> to
>> > > > release notes). And when it comes time to start contributing, I don't
>> > see
>> > > > having to create a JIRA account to be a big deal, especially if the
>> > > > associated overhead between JIRA and PRs is fairly minimal. It seems
>> > there
>> > > > are many benefits given the replies so far.
>> > > >
>> > > > Cheers,
>> > > > Stephen
>> > > >
>> > > > On Mon, 13 Nov 2017 at 11:28 Bhavin Thaker <bhavinthaker@gmail.com>
>> > wrote:
>> > > >
>> > > > > +1 for better [1] PR Titles. As suggested by Madan and use by
>> Spark,
>> > the
>> > > > > current PR template seems to be ignored by folks and so we may
want
>> > to
>> > > > > simplify it to:
>> > > > >
>> > > > > Q1. What changes were proposed in this pull request?
>> > > > >
>> > > > > Q2. How was this patch tested?
>> > > > >
>> > > > >
>> > > > > +1 to either [2] Jira OR [3] PR labels.
>> > > > >
>> > > > > Bhavin Thaker.
>> > > > >
>> > > > > On Fri, Nov 10, 2017 at 10:40 AM, Markus Weimer <markus@weimo.de>
>> > wrote:
>> > > > >
>> > > > > > Option 2 works for us over in REEF. We are a bit (too) religious
>> > about
>> > > > it
>> > > > > > [0], but it creates really nice commit messages[1].
>> > > > > >
>> > > > > > We require each commit message to start with a one line
summary
>> > which
>> > > > > names
>> > > > > > the JIRA in brackets and describes the change, e.g. `[REEF-1234]
>> > Added
>> > > > > > integration with mxnet`. The remainder of the commit message
is
>> > valid
>> > > > > > markdown, and they all end in a block which contains explicit
>> > > > references
>> > > > > to
>> > > > > > the JIRA and pull request:
>> > > > > >
>> > > > > > ```
>> > > > > > JIRA:
>> > > > > >   [REEF-1234](https://issues.apache.org/jira/browse/REEF-1234)
>> > > > > >
>> > > > > > Pull Request:
>> > > > > >   This closes #1234
>> > > > > > ```
>> > > > > >
>> > > > > > The hope is that this structure will eventually proof useful
in
>> > > > automated
>> > > > > > analysis. Then again, we haven't done that ever :)
>> > > > > >
>> > > > > > Markus
>> > > > > >
>> > > > > > [0]: https://cwiki.apache.org/confluence/display/REEF/
>> > Commit+Messages
>> > > > > > [1]: https://github.com/apache/reef/commits/master
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>

Mime
View raw message