While I agree in theory with all this,<div>we can improve our actual situation if we look at our resources,</div><div>time and people.</div><div><br></div><div>* Time: we don't have a schedule, then feature discussion can't start.</div>
<div>We can improve if we have a clear path and start to define what features</div><div>will be included in the next cycle. I am sure different players</div><div>right now have different ideas of what the next cycle can/should be,</div>
<div>we need a agreement and try to push together.</div><div><br></div><div>* People: We don't have enough people really,</div><div>and this situation can be temporally more difficult if some maintainer </div><div>have by example .... a baby :) (to not use the classic "Linus gets hit by a bus") </div>
<div>As a project, we need take care of the people working with us,</div><div>see at the personal situations, and the maintainers should </div><div>open the door to more people to be involved if possible.</div><div>For a long time, we had only Simon and Sascha doing sugar maintainership,</div>
<div>and was not enough, now we have Simon and Manuq, </div><div>and there are too much work to do. I think we should continue</div><div>work on trying to get somebody else involved as maintainer.</div><div><br></div><div>
Gonzalo</div><div><br><div class="gmail_quote">On Mon, Mar 25, 2013 at 8:37 AM, Simon Schampijer <span dir="ltr"><<a href="mailto:simon@schampijer.de" target="_blank">simon@schampijer.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hey Daniel,<br>
<br>
thanks for the write-up!<br>
<br>
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.<br>
<br>
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.<br>

<br>
I think there are different kind of patches, let's simplify with 4 categories:<br>
<br>
[bugfix] a bugfix<br>
<br>
[feature] a Feature<br>
<br>
[feature/UI] a Feature with UI<br>
<br>
[cleanup] a cleanup/maintenance<br>
<br>
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:<br>

<br>
[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.<br>

<br>
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.<br>
<br>
<br>
[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.<br>

<br>
That might explain a general bigger resistance to features by maintainers. How can we help each other to make this a better process?<br>
<br>
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.<br>

<br>
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.<br>

<br>
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.<br>
<br>
<br>
[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.<br>

<br>
<br>
[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.<br>

<br>
<br>
[1] <a href="http://wiki.sugarlabs.org/go/Features/Policy" target="_blank">http://wiki.sugarlabs.org/go/<u></u>Features/Policy</a><div class="im"><br>
<br>
<br>
On 03/24/2013 08:30 PM, Daniel Narvaez wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hello,<br>
<br>
I think patch reviews are taking too long. It's probably a well known<br>
issue but I will give a couple of examples as a "proof".<br>
<br>
Online services patches, unreviewed  for more than one month<br>
<a href="http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041808.html" target="_blank">http://lists.sugarlabs.org/<u></u>archive/sugar-devel/2013-<u></u>February/041808.html</a><br>
Unreviewed for more than one month<br>
</blockquote>
<br></div>
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.<br>
<br>
Here applies as well what I said with high level descriptions of features and the feature process [2].<br>
<br>
[2] <a href="http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041810.html" target="_blank">http://lists.sugarlabs.org/<u></u>archive/sugar-devel/2013-<u></u>February/041810.html</a><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Various patches, including automated testing stuff<br>
<a href="http://lists.sugarlabs.org/archive/sugar-devel/2012-December/041218.html" target="_blank">http://lists.sugarlabs.org/<u></u>archive/sugar-devel/2012-<u></u>December/041218.html</a><br>
Unreviewed for more than three months<br>
</blockquote>
<br></div>
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).<div class="im">
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think this situation is seriously hurting the project, in several ways:<br>
</blockquote>
<br></div>
Agreed.<div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* It makes hacking on sugar not fun at all. I honestly even lost<br>
interest in the patches I sent at this point.<br>
* It discourages new contributors. I know maintainers care about the<br>
patches I've been mentioning and I'm pretty confident they are good<br>
enough to be considered if not merged. It's just matter of priorities<br>
and lack of time. But less experienced contributors might think the<br>
project doesn't care about their contributions or they are not<br>
considered even worth an answer.<br>
* It keeps out the the tree code which could benefit the project. If<br>
my automated tests patches was in perhaps we could ask people to write<br>
tests for their code in reviews. If online services patches was in<br>
perhaps other people could build cool stuff on the top of that API.<br>
* It slows down the development peace. I reallly think active<br>
development brings more development because it makes the project more<br>
attractive to hackers.<br>
* By discouraging people to contribute we make it impossible to get<br>
more "maintainers" on board, which of course makes the problem only<br>
worst.<br>
* It blocks people which are persisent enough to not get discouraged.<br>
I don't know how many times I holded back writing a patch because I<br>
didn't want to add of my 50+ queue of patches to be reviewed. It's<br>
just too much of an hassle to manage a lot of new code outside of the<br>
tree for long.<br>
<br>
Here is my proposal to try to improve the situation.<br>
<br>
* Let's separate the maintainer from the reviewer roles. Maintainers<br>
should be very expert because ultimately the future of the project is<br>
in their hands. Those kind of people are very rare. Though I think<br>
there are many more people that could do reviews on areas of the code<br>
they feel comfortable with.<br>
</blockquote>
<br></div></div>
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.<div class="im">
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* The final word would still all be in the hands of maintainers. If<br>
they don't like a patch they override reviewers opinion. If they want<br>
more time to look at the patch or someone else to take a look, they<br>
can just ask that. They also are the ones that choose reviewers. But<br>
if a reviewer approves a patch and maintainers don't complain, then<br>
the patch lands.<br>
</blockquote>
<br></div>
Sounds good.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* By having not-yet-super-experts doing reviews we are maybe going to<br>
lower quality a bit. But there are other ways to ensure quality that<br>
doesn't slow down development as much as depending on a very small<br>
group of people to review any code change. Automated code checking<br>
(pep8 and pyflakes are very useful and easy to setup), automated<br>
testing, human testing. Honestly our attempt to ensure quality at<br>
review time is not even being very successful, we have a lot of more<br>
bugs than we should have.<br>
</blockquote>
<br></div>
+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.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So more in concrete what I'm proposing is:<br>
<br>
* We nominate Manuel and Simon maintainers of glucose as a whole. Just<br>
a cleanup while we are making changes... They are maintainers of all<br>
the modules anyway, it doesn't make a lot of sense.<br>
</blockquote>
<br></div>
Yes.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* Maintainers makes a (long enough!) list of reviewers and update it<br>
when new "experts" shows up. I don't think they need to per module,<br>
they can just review what they are comfortable to. The reviewers<br>
approve and, if necessary, land patches.<br>
</blockquote>
<br></div>
Good.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* We go back requiring all the patches to be posted on the mailing<br>
list (that's still the officlal policy but in practice a lot of code<br>
is going through trac these days). This is necessary so that<br>
maintainers can see all that is being reviewed and can comment if<br>
required.<br>
</blockquote>
<br></div>
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.<br>
<br>
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.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* We setup automated pep8 and pyflakes for all the modules. We start<br>
pretending unit/integration tests to go with patches whenever it's<br>
possible (UI bits are going to require some more infrastructure before<br>
they are possible, we can get there gradually). We start getting some<br>
human testing on master.<br>
</blockquote>
<br></div>
Yes, sounds good!<br>
<br>
<br>
Thanks again,<br>
   Simon<div class="HOEnZb"><div class="h5"><br>
<br>
______________________________<u></u>_________________<br>
Sugar-devel mailing list<br>
<a href="mailto:Sugar-devel@lists.sugarlabs.org" target="_blank">Sugar-devel@lists.sugarlabs.<u></u>org</a><br>
<a href="http://lists.sugarlabs.org/listinfo/sugar-devel" target="_blank">http://lists.sugarlabs.org/<u></u>listinfo/sugar-devel</a><br>
</div></div></blockquote></div><br></div>