brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Duncan Johnston Watt <duncan.johnstonw...@cloudsoftcorp.com>
Subject Re: Faster PR reviews
Date Tue, 02 May 2017 14:50:05 GMT
Hi Alex

This is probably covered already but I guess there needs to be an impact
assessment (by submitter?) before something is waved through by default.

Best

Duncan

On 2 May 2017 at 06:52, Alex Heneveld <alex.heneveld@cloudsoftcorp.com>
wrote:

>
> Hi Brooklyners-
>
> As many of you know, my YOML PR #363 [1] has been open for a while. This
> sets up a foundation for giving better documentation and feedback and
> hugely simplifying how we do our parsing.  However it's a very big PR.  I'm
> eager to have people spend some time using it and ideally extending it --
> but here I wanted to raise a meta-question:
>
>
> *W**hat should we do with a PR when we aren't able to review things in as
> much depth as we'd like?*
>
>
> One option -- call it (A) -- is to say if we can't review things
> thoroughly in a reasonable timeframe, we do a lighter review and if the PR
> looks promising and safe we merge it.
>
> The other option -- call it (B) -- is to leave PRs open for as long as it
> takes for us to do the complete review.
>
> I think most people have been approaching this with a mindset of (B), and
> while that's great for code quality and shared code understanding, if we
> can't deliver on that quickly, it's frankly anti-social.  The contributor
> has to deal with merge conflicts (and the rudeness of his or her
> contribution being ignored), and Brooklyn loses velocity.  My PR is an
> extreme example but many have been affected by slow reviews, and I think
> the expectation that reviews have to be so thorough is part of the
> problem:  it even discourages reviewers, as if you're not an expert in an
> area you probably don't feel qualified to review.
>
> We have good test coverage so product risk of (A) is small, and we have
> great coders so I've no worry about us being able to solve problems that
> (A) might introduce.  We should be encouraging reviewers to look at any
> area, and we need to solve the problem of slow reviews.
>
>
> *I propose that the**standard we apply is that we quickly either merge PRs
> or identify what the contributor needs to resolve.
>
>
> *I'm all for thorough reviews and shared understanding, but if we can't do
> this quickly I suggest we are better to sacrifice those things rather than
> block contributions, stifle innovation, and discourage reviews by insisting
> on a standards that we struggle to sustain.
>
> As a general rule of thumb, maybe something like:
>
> (1) After 7 days of no activity on a PR we go with an "eyeball test";
> unless the following statement is untrue we say:
>
> /I haven't done as much review as I'd like, but the code is clearly
> helpful, not risky or obviously wrong  or breaking compatibility, it has
> good test coverage, and we can reasonably expect the contributor or
> committers to maintain it.  Leaving open a bit longer in case someone else
> wants to review more but if nothing further in the next few days, let's
> merge.
>
> /(If there are committers who are likely to be specifically interested,
> call them out as CC.)
>
> (2) After 3 more days, if no activity, merge it.
>
> And we encourage _anyone_ to review anything.  If the above response is
> the baseline, everyone in our community is qualified to do it or better and
> we'll be grateful!
>
> Best
> Alex
>
>
> [1]  https://github.com/apache/brooklyn-server/pull/363
>
>


-- 

Duncan Johnston-Watt

Founder & Chief Executive Officer

Phone: +44 777 190 2653 | Skype: duncan_johnstonwatt

Twitter: @duncanjw <https://twitter.com/duncanjw> | LinkedIn:
https://linkedin.com/in/duncanjohnstonwatt

[image: Cloudsoft Logo.jpg] <https://cloudsoft.io/>

Stay up to date with everything Cloudsoft:

[image: Twitter_Logo_White_On_Blue.png] <https://twitter.com/cloudsoft> [image:
YouTube-social-icon_red_48px.png]
<https://www.youtube.com/channel/UCpbLhvXrYWz8B_osUX6rn0Q>

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