[Sugar-devel] Proposal on how to speed up patch reviews

[Simon Schampijer]

Hey Daniel,

thanks for the write-up!

It is true that patch review, especially of non-bugfix patches is slow 
and can be sometimes frustrating for all parties involved, the reviewer, 
the maintainer and the submitter.

To improve that situation, I think we have to put some lights on all 
those roles and I think often the situation of the maintainer is not 
understood well enough. I can at least say that we had this discussions 
for the last years in Sugar, with different maintainers and I have seen 
it happening in many other projects as well. It is a known issue, and it 
is not an easy one, never less I think we can improve.

I think there are different kind of patches, let's simplify with 4 
categories:

[bugfix] a bugfix

[feature] a Feature

[feature/UI] a Feature with UI

[cleanup] a cleanup/maintenance

There are different kind of roles in the process and each will act 
differently based on his needs and duties. There are the submitters, the 
reviewers (can be either the maintainer, or someone else from the 
community) and the maintainers. Let's step through the categories and 
talk about the different roles:

[bugfix] A bugfix is the simplest case. The submitter is interested in 
solving the specific issue he is working on. It is not hard to find a 
reviewer or tester. Either someone from the community that has been 
annoyed by the same issue or the maintainer himself because he is 
interested in seeing the issue solved, his interest is a working code 
base in the end. Here it is easy as well for a maintainer to trust 
another reviewer and acknowledge based on his positive review.

In my experience those patches are not laying around for long if there 
is not a technical blocker in the patch itself. Evaluation is relatively 
easy here.


[feature] When it gets to Features things get more tricky. For a Feature 
first of all the high level goals are important: what need does the 
Feature address, is it wanted by the community, is the technical 
approach taken a good one, basically the maintainer has to decide if it 
is worth taking on maintainership of this feature or not. In the end it 
might be him who has to deal with arising bug fixes and who is blamed if 
the software is not a solid product.

That might explain a general bigger resistance to features by 
maintainers. How can we help each other to make this a better process?

First of all, it is important to know the high level goals of a feature. 
The feature process [1] in place actually helps in that regard, the 
maintainer can do the evaluation based on the community feedback and the 
high level goals and if those are accepted can do the actual review of 
the code.

Getting approval for a feature does take time, and the review itself 
often as well, this should be counted in as well from all the sides, 
especially when it is the desire to see a specific feature being present 
in a release.

Setting goals at the beginning of a release and having an accepting 
feature period will help here as well, actually all of this is in place 
already.


[feature/UI] All what have been said above applies to the feature that 
adds UI as well. I have separated that item because it adds another 
complexity. We have the UI process for this (as written in the feature 
policy [1]). Basically it adds more care taking of all the roles involved.


[cleanup] These are simple as well and can be acknowledged without much 
thinking. In my opinion, maintainers should be able to do those without 
the need of a review, if uncertain or bigger parts are touched they can 
discuss the items with the other maintainers.


[1] http://wiki.sugarlabs.org/go/Features/Policy


On 03/24/2013 08:30 PM, Daniel Narvaez wrote:
> Hello,
>
> I think patch reviews are taking too long. It's probably a well known
> issue but I will give a couple of examples as a "proof".
>
> Online services patches, unreviewed  for more than one month
> http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041808.html
> Unreviewed for more than one month

This one is part of the [feature] category. It can be mostly explained 
with the maintainers having their heads still in bug fixing for 0.98.

Here applies as well what I said with high level descriptions of 
features and the feature process [2].

[2] http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041810.html

> Various patches, including automated testing stuff
> http://lists.sugarlabs.org/archive/sugar-devel/2012-December/041218.html
> Unreviewed for more than three months

This is nearly category [cleanup]. We should not block here on a in 
depth review when things pass the usual tests (pep8 etc) and the high 
level approach is accepted (taking for granted that the maintainer is 
fixing up the issues that might arise himself).

> I think this situation is seriously hurting the project, in several ways:

Agreed.

> * It makes hacking on sugar not fun at all. I honestly even lost
> interest in the patches I sent at this point.
> * It discourages new contributors. I know maintainers care about the
> patches I've been mentioning and I'm pretty confident they are good
> enough to be considered if not merged. It's just matter of priorities
> and lack of time. But less experienced contributors might think the
> project doesn't care about their contributions or they are not
> considered even worth an answer.
> * It keeps out the the tree code which could benefit the project. If
> my automated tests patches was in perhaps we could ask people to write
> tests for their code in reviews. If online services patches was in
> perhaps other people could build cool stuff on the top of that API.
> * It slows down the development peace. I reallly think active
> development brings more development because it makes the project more
> attractive to hackers.
> * By discouraging people to contribute we make it impossible to get
> more "maintainers" on board, which of course makes the problem only
> worst.
> * It blocks people which are persisent enough to not get discouraged.
> I don't know how many times I holded back writing a patch because I
> didn't want to add of my 50+ queue of patches to be reviewed. It's
> just too much of an hassle to manage a lot of new code outside of the
> tree for long.
>
> Here is my proposal to try to improve the situation.
>
> * Let's separate the maintainer from the reviewer roles. Maintainers
> should be very expert because ultimately the future of the project is
> in their hands. Those kind of people are very rare. Though I think
> there are many more people that could do reviews on areas of the code
> they feel comfortable with.

That sounds good to me. We actually are doing this more or less already. 
We can maybe make this more explicit and foster that principle. 
Especially for bug fixes I do not see a reason why I should not merge a 
patch that has r+/t+ from a trusted person if there is not an obvious issue.

> * The final word would still all be in the hands of maintainers. If
> they don't like a patch they override reviewers opinion. If they want
> more time to look at the patch or someone else to take a look, they
> can just ask that. They also are the ones that choose reviewers. But
> if a reviewer approves a patch and maintainers don't complain, then
> the patch lands.

Sounds good.

> * By having not-yet-super-experts doing reviews we are maybe going to
> lower quality a bit. But there are other ways to ensure quality that
> doesn't slow down development as much as depending on a very small
> group of people to review any code change. Automated code checking
> (pep8 and pyflakes are very useful and easy to setup), automated
> testing, human testing. Honestly our attempt to ensure quality at
> review time is not even being very successful, we have a lot of more
> bugs than we should have.

+1 I would like to encourage people as well to report back when they 
tested a patch. Again here, at least for bug fixes that is a valid approach.

> So more in concrete what I'm proposing is:
>
> * We nominate Manuel and Simon maintainers of glucose as a whole. Just
> a cleanup while we are making changes... They are maintainers of all
> the modules anyway, it doesn't make a lot of sense.

Yes.

> * Maintainers makes a (long enough!) list of reviewers and update it
> when new "experts" shows up. I don't think they need to per module,
> they can just review what they are comfortable to. The reviewers
> approve and, if necessary, land patches.

Good.

> * We go back requiring all the patches to be posted on the mailing
> list (that's still the officlal policy but in practice a lot of code
> is going through trac these days). This is necessary so that
> maintainers can see all that is being reviewed and can comment if
> required.

This works if we make our process fast enough :) The issue here is that 
we do not have a good way of tracking unmerged patches. If we merge 
quickly that works.

As well, we still have the bug tracker. When we go into bug fixing mode 
things get messed up. I still do not have a good answer to that.

> * We setup automated pep8 and pyflakes for all the modules. We start
> pretending unit/integration tests to go with patches whenever it's
> possible (UI bits are going to require some more infrastructure before
> they are possible, we can get there gradually). We start getting some
> human testing on master.

Yes, sounds good!


Thanks again,
    Simon