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

[Daniel Narvaez]

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

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

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

* 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.
* 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.
* 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.

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.
* 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.
* 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.
* 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.

Hope it helps.

-- 
Daniel Narvaez