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

[Walter Bender]

On Mon, Mar 25, 2013 at 7:37 AM, Simon Schampijer <simon at schampijer.de> wrote:
> 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

For the record, [3], [4] were sent to the devel list within 6 hours of
your request [2] :P

[3] http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041817.html
[4] http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041820.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
>
>
> _______________________________________________
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel



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