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
