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

[Walter Bender]

On Sun, Mar 24, 2013 at 3:30 PM, Daniel Narvaez <dwnarvaez at gmail.com> 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
>
> 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
> _______________________________________________
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel

All seems reasonable to me.

-walter

-- 
Walter Bender
Sugar Labs
http://www.sugarlabs.org