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

[Simon Schampijer]

On 03/25/2013 12:46 PM, Walter Bender wrote:
> 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

Ahh, excellent. Didn't see them. In any case, was just an example of a 
typical case.

Regards,
    Simon