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

[Simon Schampijer]

On 03/25/2013 09:32 PM, Daniel Narvaez wrote:
> Forgot to reply all...
>
>
> ---------- Forwarded message ----------
> From: Daniel Narvaez <dwnarvaez at gmail.com>
> Date: 25 March 2013 21:12
> Subject: Re: [Sugar-devel] Proposal on how to speed up patch reviews
> To: Simon Schampijer <simon at schampijer.de>
>
>
> On 25 March 2013 12:37, Simon Schampijer <simon at schampijer.de> wrote:
>> 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.
>
> In my experience Sugar has been much worst than both GNOME and mozilla
> though. I know it's not easy but we should keep trying.
>
> (I hope it's absolutely clear that I'm not blaming anyone for the
> situation, just pointing out the importance of fixing it or at least
> trying to)
>
>> [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.
>
> I had at least one obvious bug fix patch unreviewed for months. Maybe
> I was unlucky. Anyway I agree this is the easiest of the cases and
> where things work best.

That is bad of course. Could have been several reasons. Maybe the 
decoupling of patches and the bug tracker, maybe just felt of the 
table... Sometimes a ping is valid option. But yes, the easiest area to 
solve.

>> [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.
>
> While I agree with you in general, I think maybe we are exagerating a
> bit the responsibility of the maintainers a bit. I tend to think it's
> the whole community which will get the blame if things goes wrong...
> Maintainers have of course a very important role, but they should not
> feel like they alone into this.

 From my experience the work on a feature and the polish of it gets 
often underestimated. The first 90% are done in 10% of the time the last 
10% are done in 90% of the time. I would like to establish a sense of 
the work needed to finish a feature, not to make people afraid of 
starting to work on features but to be realistic.

>> That might explain a general bigger resistance to features by maintainers.
>> How can we help each other to make this a better process?
>
> I think narrowing the focus is the best we can do, but I'll elaborate
> more about that while answering Gonzalo email.
>
>> [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.
>
> Even if we go with my propiosal, I think maintainers should keep a
> strong supervision role on features, UI or not. I'd say it's
> responsibility of the reviewer to make sure the maintainer is happy in
> this regard... we should add it to our review checklist (well we don't
> have one yet afaik, but we should).

Yes, from my experience, the UI review part of a Feature takes even 
longer than the code review. First of all we do not have as many 
designers as people who can do review and good consistent UI is not 
easy. Gary and Manuel are currently helping on that end. Probably good 
to hear them, if they have anything to add that could help to make 
things go more smooth.

>>> 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].
>
> In an ideal world, a reviewer which is not busy with 0.98 should have
> given a first pass to the patches and at the same time started a
> discussion, involving the maintainer, about the oppurtunity of adding
> such a feature etc.

Good.

>>> * 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.
>
> There a couple of important differences compared to what we are doing:
>
> * The reviewers are explicitly entrusted by the maintainers. So they
> know if they review the patches will most likely go in. It's often not
> very motivating to do reviews without being able to approve.
> * The reviewers takes care of merging, so there is no blocking on the
> maintainers (if not for an high level discussion on feature changes).

Ok.

>>> * 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.
>
> I thought about this a lot in the past and honestly I don't have a
> good answer either. I think it's less urgent than adding people that
> can do reviews. Hopefully we will figure it out at some point!

Sounds good to me!
    Simon