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