geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jianxia Chen <jche...@apache.org>
Subject Re: [discuss] Should we evaluate at commit messages as part of PR review?
Date Thu, 13 Sep 2018 00:15:17 GMT
There is a good article on this topic that Anthony recommended a while ago:
http://chris.beams.io/posts/git-commit/

Jianxia

On Wed, Sep 12, 2018 at 4:30 PM Michael Stolz <mstolz@pivotal.io> wrote:

> +1 to descriptive commit messages
>
> Done well they will save time on every post commit access to that commit.
>
> --
> Mike Stolz
> Principal Engineer - Gemfire Product Manager
> Mobile: 631-835-4771
>
> On Sep 12, 2018 4:15 PM, "Alexander Murmann" <amurmann@pivotal.io> wrote:
>
> > While our wiki page primarily calls out the 50/74 rule which is
> important,
> > my bigger concern is in that I'd love to see commit messages that explain
> > why a change was made. Sometimes that information is in the reference
> JIRA
> > ticket, but not always. Especially with bug fixes we tend to not explain
> > what actually caused the bug and how the code changes address it. It's
> also
> > nice to minimize moving back and forth between editor and browsing JIRA
> and
> > even better not to have to read 20+ messages long comment threads about a
> > bug in JIRA to find out what the problem was. No hard rule can make sure
> we
> > are doing that right, only human reviewers and care can. I don't think
> it's
> > actually any extra work, since we are already reading the PR description
> > and commit messages when we are doing a PR review. How else can you
> > understand the PR? In fact better commit messages should save time here
> and
> > not steal it.
> >
> > I totally agree that we don't always need a long text. "Fix typo in XYZ
> > output" is totally fine. Both the why and what are obvious.
> >
> > On Wed, Sep 12, 2018 at 4:07 PM, Helena Bales <hbales@pivotal.io> wrote:
> >
> > > I'm a fan of having useful commit messages. I would prefer this to be
> > > another checkbox in our list of 6 things to do for Pull Requests as
> > opposed
> > > to something that is strictly enforced. There are cases where a simple
> > > one-liner commit message is enough. I personally use commit messages
> > often,
> > > even when looking back at my own work. Additionally, I think it is a
> > useful
> > > exercise as it forces the dev to think back on the original problem and
> > > think about how the solution addresses that.
> > >
> > > tl;dr -- +1 for commit messages
> > >
> > > ~Helena
> > >
> > > On Wed, Sep 12, 2018 at 11:50 AM, Pulkit Chandra <pchandra@pivotal.io>
> > > wrote:
> > >
> > > > Have we thought about git hooks as a way to enforce policy
> > > > https://git-scm.com/book/en/v2/Customizing-Git-An-Example-
> > > > Git-Enforced-Policy
> > > > ?
> > > >
> > > > *Pulkit Chandra*
> > > >
> > > >
> > > > On Wed, Sep 12, 2018 at 2:46 PM Alexander Murmann <
> amurmann@pivotal.io
> > >
> > > > wrote:
> > > >
> > > > > Hi everyone,
> > > > >
> > > > > We have a wiki page
> > > > > <https://cwiki.apache.org/confluence/display/GEODE/
> > > Commit+Message+Format
> > > > >
> > > > > that discusses why good commit messages matter and links to a even
> > > better
> > > > > article on the topic. In addition to what's described in those
> > > documents,
> > > > > better commit messages also would make it easier to have good PR
> > > > messages.
> > > > > Good commit and PR messages also provide more context to the
> reviewer
> > > who
> > > > > in turn now can do a better job at reviewing the pull request.
> > > > >
> > > > > Looking at our git log gives me the impression that we aren't
> always
> > > > living
> > > > > up to that standard. In fact we frequently aren't even close.
> > > > >
> > > > > I propose taking clear and well formatted commit messages into
> > account
> > > as
> > > > > part of our PR review process. Lacking commit messages can be just
> as
> > > bad
> > > > > as bad naming in our code.
> > > > >
> > > > > Thoughts?
> > > > >
> > > >
> > >
> >
>

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