On 03/25/2013 09:32 PM, Daniel Narvaez wrote:
Forgot to reply all...
---------- Forwarded message ----------
From: Daniel Narvaez <dwnarv...@gmail.com>
Date: 25 March 2013 21:12
Subject: Re: [Sugar-devel] Proposal on how to speed up patch reviews
To: Simon Schampijer <si...@schampijer.de>
On 25 March 2013 12:37, Simon Schampijer <si...@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
_______________________________________________
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
