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

Gonzalo Odiard gonzalo at laptop.org
Mon Mar 25 08:27:25 EDT 2013


While I agree in theory with all this,
we can improve our actual situation if we look at our resources,
time and people.

* Time: we don't have a schedule, then feature discussion can't start.
We can improve if we have a clear path and start to define what features
will be included in the next cycle. I am sure different players
right now have different ideas of what the next cycle can/should be,
we need a agreement and try to push together.

* People: We don't have enough people really,
and this situation can be temporally more difficult if some maintainer
have by example .... a baby :) (to not use the classic "Linus gets hit by a
bus")
As a project, we need take care of the people working with us,
see at the personal situations, and the maintainers should
open the door to more people to be involved if possible.
For a long time, we had only Simon and Sascha doing sugar maintainership,
and was not enough, now we have Simon and Manuq,
and there are too much work to do. I think we should continue
work on trying to get somebody else involved as maintainer.

Gonzalo

On Mon, Mar 25, 2013 at 8: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<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<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<http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041810.html>
>
>
>  Various patches, including automated testing stuff
>> http://lists.sugarlabs.org/**archive/sugar-devel/2012-**
>> December/041218.html<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 <Sugar-devel at lists.sugarlabs.org>
> http://lists.sugarlabs.org/**listinfo/sugar-devel<http://lists.sugarlabs.org/listinfo/sugar-devel>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20130325/66f7b7e5/attachment.html>


More information about the Sugar-devel mailing list