+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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >> 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 <[email protected]> >> > 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 <[email protected]> >> > 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 >> > > > > > >> > > > > >> > > > >> > > >> > >>
