subject:"review stop\-energy \(was 24hour review\)"


On 7/11/13 2:41 PM, Chris Pearce wrote:

Maybe you need to start farming out reviews to other/newer members of
the teams you work on?


Oh, that's been happening.  A lot.

-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Axel Hecht


On 7/11/13 8:24 PM, Jeff Walden wrote:

On 07/11/2013 03:09 AM, Nicholas Nethercote wrote:

On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden  wrote:


Establishing one-day turnaround time on reviews, or on requests, would require 
a lot more polling on my review queue.


You poll your review queue?  Like, by visiting your Bugzilla
dashboard, or something like that?  That's *awful*.

I personally use a push notification system called "email with
filters".  Well, strictly speaking it's poll-like because I have to
check my "high priority bugs" folder, but I do that anyway multiple
times per day so I'm unlikely to take more than an hour or two (while
working) to notice a review request.


I have 
https://bugzilla.mozilla.org/request.cgi?requestee=jwalden%2Bbmo%40mit.edu&do_union=1&group=type&action=queue
 open in a browser tab and check it from time to time.  I don't see how that's any 
different from a mail-filtering folder except in terms of the UI.  They're both polling, as 
you note.  :-)

Jeff



I wish I could watch more than one requestee in one page. I actually 
have a tab with a history of three bugzilla accounts' requests page, and 
go back and forth and reload.


Axel
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Chris Pearce


On 7/11/2013 8:55 AM, Boris Zbarsky wrote:

On 7/11/13 11:37 AM, Robert O'Callahan wrote:

While I think your observations are useful, I think (hope!) you are a
massive outlier


Somewhat.  My list was based on not only what makes my reviews slow 
but what I've heard people mention as making reviews slow.


I do agree we shouldn't design a policy based on my review queue. ;)


Maybe you need to start farming out reviews to other/newer members of 
the teams you work on? We've done that in media; giving reviews to the 
newer members of the team has been a great way to spread the load, and 
upskill the other team members.


Obviously some patches need the attention of an experienced hacker like 
yourself, but style nitsand mechanical errors etc can be picked up a 
less experienced reviewer. You could always look for the big issues, 
feedback+/- and then reassign the review request to another reviewer.



Chris P.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Jeff Walden

On 07/11/2013 03:09 AM, Nicholas Nethercote wrote:
> On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden  wrote:
>>
>> Establishing one-day turnaround time on reviews, or on requests, would 
>> require a lot more polling on my review queue.
> 
> You poll your review queue?  Like, by visiting your Bugzilla
> dashboard, or something like that?  That's *awful*.
> 
> I personally use a push notification system called "email with
> filters".  Well, strictly speaking it's poll-like because I have to
> check my "high priority bugs" folder, but I do that anyway multiple
> times per day so I'm unlikely to take more than an hour or two (while
> working) to notice a review request.

I have 
https://bugzilla.mozilla.org/request.cgi?requestee=jwalden%2Bbmo%40mit.edu&do_union=1&group=type&action=queue
 open in a browser tab and check it from time to time.  I don't see how that's 
any different from a mail-filtering folder except in terms of the UI.  They're 
both polling, as you note.  :-)

Jeff
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Jet Villegas

I may have a skewed view of things, but I personally have not had problems 
getting prompt code reviews. I also don't have a problem talking to my 
reviewers about what I'm hacking on. I'm hesitant to throw a bunch of 
technology at this problem, if it's a social issue. Code reviews are a 
conversation and more code has to come with more conversations. 

Talk to each other, people!

We'll cover this topic at the next Rendering Meeting. Let's have a look at the 
bugs sitting in review and reassign reviewers as needed. We need to grow more 
reviewers, and the only way I know to do that is to trust new people with 
reviewing code (and check their work.)

--Jet
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Milan Sreckovic


That last thing was another item I found useful in the previous life.  When 
requesting a review from somebody, people could see "this person currently has 
X items in their review queue".  You can ignore that information, but it's 
there and it may help.  It's still probably simpler for the reviewer to know 
they're too busy, notify the author, and help them find somebody else if they 
can't agree on the timeframe.


On 2013-07-11, at 11:37 , Robert O'Callahan  wrote:

> While I think your observations are useful, I think (hope!) you are a
> massive outlier and I don't think we should design a policy based on the
> assumption that your review commitments are in any way normal.
> 
> I would be totally OK with a policy that explicitly applies to everyone
> except you. Or, we could parameterize it with a dynamic list of exceptions
> who are known to be overloaded with reviews. In fact, making that list
> explicit and publishing it would let people know not to request reviews
> from people on that list except as a last resort (which is, in fact, how I
> treat you and dbaron).
> 
> Rob
> -- 
> Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
> le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
> stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
> 'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
> waanndt  wyeonut  thoo mken.o w  *
> *
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Mark Côté

On 2013-07-11 7:59 AM, Gervase Markham wrote:
> On 09/07/13 21:29, Chris Peterson wrote:
>> I've seen people change their Bugzilla name to include a comment about
>> being on PTO. We should promote this practice. We could also add a
>> Bugzilla feature (just a simple check box or a PTO date range) that
>> appends some vacation message to your Bugzilla name.
> 
> Hey, if we had a PTO app that tracked all absences, we could integrate
> with it...
> 
> 
>> Some review tools track review comments, so checking subsequent patches
>> is easier. A couple months, I heard discussion about an investigation of
>> Review Board for Bugzilla. Is anyone still investigating Review Board?
> 
> Yes; it's on the BMO roadmap to be integrated; I believe its even being
> worked on now. CCing glob.

To be clear and to correct something I said earlier:  we are standing up
a somewhat independent RB service this quarter, which will be connected
to Bugzilla's auth and will know about some common repos but nothing
more.  RB seems great but I don't think we've conclusively decided that
it's the best solution; we probably need to experiment with it a bit to
make sure it works best for us.

Mark

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread L. David Baron

On Thursday 2013-07-11 00:14 -0700, Robert O'Callahan wrote:
> We can't have a rigid rule about 24 hours. Someone requesting a review from
> me on Thursday PDT probably won't get a response until Monday if neither of
> us work during the weekend.
> 
> But I think it's reasonable to expect developers to process outstanding
> review requests (and needinfos) at least once every regular work day.
> Processing includes leaving a comment with an ETA.

So, partly, I'm really bad at figuring out ETAs for small tasks,
since I find the priority order of small tasks to be relatively
dynamic, given how often small things come up.  For example, reading
and responding to this thread (something I've spent at least 15
minutes on so far this morning, and it'll probably end up being more
than 30, which is probably 10% of my non-meeting working day today).
Should I have prioritized that above doing code reviews, or should I
come back to this thread and give you my thoughts in 3 weeks (as I'm
currently doing on the prefixing policy thread, which I feel
requires more thought)?

I spend a pretty big portion of my time on things that come up at
the last minute:  questions from colleagues, discussions on lists
(Mozilla lists and standards lists, etc.).  (Another interesting
question: should I prioritize questions / needinfos from people in
the *middle* of writing a patch over code reviews which are at the
*end* of writing a patch?  Right now I think I sometimes do, and
sometimes treat them equally.)

Like Boris, I feel guilty about not getting to reviews, and I feel
like I'm bad at figuring out how to prioritize them.

I suppose what leaving an ETA would do is force me to try to stick
to what I've promised, which in turn means doing code reviews rather
than doing things like reading email or responding to this thread.

-David

-- 
𝄞   L. David Baron http://dbaron.org/   𝄂
𝄢   Mozilla   http://www.mozilla.org/   𝄂
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/11/13 11:37 AM, Robert O'Callahan wrote:

While I think your observations are useful, I think (hope!) you are a
massive outlier


Somewhat.  My list was based on not only what makes my reviews slow but 
what I've heard people mention as making reviews slow.


I do agree we shouldn't design a policy based on my review queue.  ;)


I would be totally OK with a policy that explicitly applies to everyone
except you. Or, we could parameterize it with a dynamic list of exceptions
who are known to be overloaded with reviews.


So the thing is, I'm not _always_ overloaded with reviews.  I generally 
keep on top of them unless I take time off or several large patches end 
up in my review queue at once.


And I think that applies to most people who are trying to do their 
reviews...  The problem is that right now there is no way to mark 
yourself as "temporarily less responsive on reviews than normal", 
whether it's because you're on vacation or because you have a 500KB 
patch to review that will take all week.  And then reviews pile on.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

The way I work is that review and needinfo requests go to my inbox and I
process them with high priority. I can and do ignore them there temporarily
if I need to. Given I use my inbox as a to-do list, that approach seems
perfect for me.

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

On Thu, Jul 11, 2013 at 6:23 AM, Justin Lebar wrote:

> Someone who usually has a long review queue has told me that he "hates"
> reviewing code.  I realized that we don't really have a place at Mozilla
> for
> experienced hackers who don't want to do reviews.  Should we?
>

I think someone can "hate doing reviews" and also "want to do reviews"
because they're important. I don't know anyone who loves doing reviews.

As another data point, someone told me recently that he's trying not to
> become
> a peer of the modules he's working on, because he doesn't want to review
> code.
>

If someone who can do reviews is avoiding them, they're pushing workload
onto their peers who probably don't enjoy it much more. That's antisocial
and unacceptable IMHO.

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

While I think your observations are useful, I think (hope!) you are a
massive outlier and I don't think we should design a policy based on the
assumption that your review commitments are in any way normal.

I would be totally OK with a policy that explicitly applies to everyone
except you. Or, we could parameterize it with a dynamic list of exceptions
who are known to be overloaded with reviews. In fact, making that list
explicit and publishing it would let people know not to request reviews
from people on that list except as a last resort (which is, in fact, how I
treat you and dbaron).

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Ehsan Akhgari


On 2013-07-10 6:27 PM, Mark Côté wrote:

On 2013-07-10 2:18 PM, Boris Zbarsky wrote:

On 7/10/13 1:58 PM, Mark Côté wrote:

The BMO team is again considering switching to ReviewBoard, which should
fix this problem


How does ReviewBoard address this?

Again, what we have in the bug is diff 1 against changeset A and diff 2
against changeset B that incorporates the review changes.  What the
reviewer would like to see is a diff from 1 rebased against B to 2.

I guess that's possible if the system tries to automatically rebase diff
1 against changeset B, but if the automatic rebase fails you still fail...


True, sorry, I meant only the part about it having access to source
control.  Apologies for the diversion.


I've talked to the BMO folks about this before.  ReviewBoard doesn't 
know what the base revision of patches are, so it will not be helpful. 
It's just a fancier splinter.  To do this properly we need to push 
commits/changesets somewhere instead of attaching textual diffs, and 
unfortunately that doesn't seem to be a problem that the ReviewBoard 
switch is trying to solve.


Ehsan

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/11/13 9:23 AM, Justin Lebar wrote:

One thing I've been thinking about is /why/ people are slow at reviews.


Here are some possible reasons I've encountered myself:

1)  Feeling overwhelmed because you have too many reviews pending and 
therefore just staying away from your list of reviews.  Note that this 
can be self-perpetuating.


2)  Feeling like you have to do your reviews in some sort of FIFO order, 
so that a review for a large patch (which takes a long time because 
that's just how large patches work) will block possibly-quick reviews 
for smaller patches.


3)  Feeling like you want to do your reviews in some sort of 
most-expeditious order, so that a constant stream of small patches 
delays review of a big patch for weeks or months.


4)  Feeling that you need multiple hours of uninterrupted time, probably 
over several days for review of a complicated patch and not being able 
to find it due to the scattering of meetings, people asking you 
questions, smaller reviews, etc that you're also doing.  Note that 
anyone who has no time to code because of reviews probably also has no 
time to do big complicated reviews, for the same reasons!


5)  Simply having too high a review load.  Anecdotally, it typically 
takes me until sometime Tuesday afternoon at best to catch up on all the 
review requests that come in between end-of-work Friday and Monday morning.


6)  Just disliking doing reviews (but more on this below).

7)  Wanting to completely focus on some other complicated task for a few 
days ... or weeks.


It would be interesting to see what other reasons there are and what the 
relative frequencies are.



Someone who usually has a long review queue has told me that he "hates"
reviewing code.


Reviewing code is just not all that fun.  You have to tell people they 
did something wrong, often with some fairly arbitrary (in the grand 
scheme of things; we do it so we have consistent coding style) nitpicks, 
there is not that much of a sense of accomplishment at the end.  It 
feels very adversarial at times.  Sometimes you learn new things in the 
process, but more often you try hard to politely tell people that 
they've screwed something up... possibly after you already told them 
that about that same thing in the previous iteration of the patch.  At 
least for me, it can get downright depressing at times.


But debugging leaks is not that fun either.  Or trying to read through 
flat profiles and speed things up.  Or hunting down a dangling pointer 
bug.  Or trying to convince people on a standards mailing list to not do 
something insane, for that matter.


Fundamentally, we feel that reviews are useful and that means they have 
to get done; the doing is not all that fun, but it's a sort of giving 
back to the community, in my view.



I realized that we don't really have a place at Mozilla for
experienced hackers who don't want to do reviews.  Should we?  Could we do this
without violating people's sense of fairness?


How do we handle this with the other unpalatable tasks above?

Informally, what I see is that they get shunted to people who are either 
not as unhappy doing them or have a stronger sense of "I don't like it, 
but it needs doing, and no one else is stepping up".  And it sure as 
heck violates people's sense of fairness.  Of course we do that with 
reviews too (a well-known technique for doing fewer reviews is just 
being slow about it so people stop asking, whereas someone who does 
reviews quickly is "rewarded" by people piling on reviews), and it 
likewise violates people's sense of fairness.


Of course the other thing I see happen with the unpalatable tasks I list 
above is that they just don't get done in many cases because no one 
steps up.  That's a bit rarer with reviews, because we don't consider 
those as optional. But it has happened: I've seen patches die because no 
one stepped up to review.


Here's another way to look at this: do these hackers want _others_ to 
review their patches?  If so, how do they expect that to happen?


Obviously everyone would prefer to work on the things they _want_ to 
work on, not necessarily the things that _need_ to be worked on.  To the 
extent that someone focuses more on the former than on the latter, 
they're shafting others whose sense of duty won't let them do that.  The 
result is that you get people who are unhappy because they never get to 
work on the things they want to or who are unhappy because they're doing 
all their "I would like to get this done" work after hours.  Or both, at 
times: only working on things that need to happen _and_ having to do it 
after hours...


And as a further note, the more reviews pile on just a small subset of 
people, the more problems 1-5 listed above manifest.  As Taras said, the 
right solution to review load is to grow more reviewers, but that's hard 
to do if people are basically shirking the responsibility.



As another data point, someone told me recently that he's trying not

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Ted Mielczarek

On 7/11/2013 9:23 AM, Justin Lebar wrote:
> One thing I've been thinking about is /why/ people are slow at reviews.
>
> Someone who usually has a long review queue has told me that he "hates"
> reviewing code.  I realized that we don't really have a place at Mozilla for
> experienced hackers who don't want to do reviews.  Should we?  Could we do 
> this
> without violating people's sense of fairness?
>
> As another data point, someone told me recently that he's trying not to become
> a peer of the modules he's working on, because he doesn't want to review code.
> Maybe we're not incentivizing people enough to be reviewers.
>
> Roc said that we've spoken with people who are slow at reviews.  Did we learn
> anything from that?  I'd expect that people are slow for different reasons.
>
> It seems backward to me to focus on a solution without first understanding the
> causes.
I can definitely sympathize with this, as the build system owner (now
peer), and test harness owner I get a lot of reviews thrown my way. I
have been able to pretty successfully grow the peer list of both modules
over the past few years, and it's worked out pretty well. One nice
benefit that people shouldn't overlook is that by becoming a peer in a
module you work in you help spread out the review load, which means that
the owner and other peers have more time to review *your* patches. This
has been really great for me in the build system, where I prioritize
reviews requests from the other peers, and in return also get quick
turnaround on patches I need review for. It's a little "tit-for-tat",
but I think it's a healthy positive feedback loop.

-Ted

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Ehsan Akhgari


On 2013-07-11 3:06 AM, Robert O'Callahan wrote:

On Wed, Jul 10, 2013 at 6:09 AM, smaug  wrote:


One thing, which has often brought up, would be to have other automatic
coding style checker than just Ms2ger.



See https://bugzilla.mozilla.org/show_bug.cgi?id=875605, which is,
ironically, waiting for review.


Anybody who knows python can help with reviews there, FWIW.

Cheers,
Ehsan

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Mark Banner

On 11/07/2013 12:59, Gervase Markham wrote:
> On 09/07/13 21:29, Chris Peterson wrote:
>> I've seen people change their Bugzilla name to include a comment about
>> being on PTO. We should promote this practice. We could also add a
>> Bugzilla feature (just a simple check box or a PTO date range) that
>> appends some vacation message to your Bugzilla name.
> 
> Hey, if we had a PTO app that tracked all absences, we could integrate
> with it...
> 

I'd rather have a custom field anyway. Sometimes I'm highly focussed on
a project for particular reason, or doing meet-up weeks. During those
times I rarely even check bugmail for my other projects.

Mark.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/11/13 7:59 AM, Gervase Markham wrote:

Hey, if we had a PTO app that tracked all absences, we could integrate
with it...



Just in case you were talking about the moco PTO app, it doesn't track 
absences for non-MoCo employees, and even for employees it does not 
track non-PTO absences (being a PTO app and all).


-Boris

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Justin Lebar

One thing I've been thinking about is /why/ people are slow at reviews.

Someone who usually has a long review queue has told me that he "hates"
reviewing code.  I realized that we don't really have a place at Mozilla for
experienced hackers who don't want to do reviews.  Should we?  Could we do this
without violating people's sense of fairness?

As another data point, someone told me recently that he's trying not to become
a peer of the modules he's working on, because he doesn't want to review code.
Maybe we're not incentivizing people enough to be reviewers.

Roc said that we've spoken with people who are slow at reviews.  Did we learn
anything from that?  I'd expect that people are slow for different reasons.

It seems backward to me to focus on a solution without first understanding the
causes.

-Justin

On Thu, Jul 11, 2013 at 3:08 AM, Robert O'Callahan  wrote:
> On Wed, Jul 10, 2013 at 3:26 PM, Justin Lebar 
> wrote:
>>
>> If I can propose something that's perhaps different:
>>
>> 1) Write software to figure out who's "slow with reviews".
>> 2) We talk to those people.
>
>
> We've done this before too.
>
> But we should just do it again --- the "definition of insanity" aphorism is
> nonsense. Repetition helps people learn.
>
> Rob
> --
> Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni le
> atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
> stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
> 'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
> waanndt  wyeonut  thoo mken.o w
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Gervase Markham

On 10/07/13 15:09, Boris Zbarsky wrote:
> And communicated via bzapi so bzexport can also warn.

BzAPI could add a flag based on a parsing of the name - but then, if
there was an accurate algorithm for parsing a name to extract absence
information, bzexport could use it directly.

Perhaps we could enhance bzexport to request the username of the user of
whom you are requesting review, and display it to you at the time of
submission? You could then notice manually if they were away.

Gerv

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Gervase Markham

On 10/07/13 23:14, Taras Glek wrote:
> I tried to capture feedback from this thread in
> https://wiki.mozilla.org/Code_Review

I just did a pass over that page to highlight the key points.

Gerv

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Gervase Markham

On 09/07/13 21:29, Chris Peterson wrote:
> I've seen people change their Bugzilla name to include a comment about
> being on PTO. We should promote this practice. We could also add a
> Bugzilla feature (just a simple check box or a PTO date range) that
> appends some vacation message to your Bugzilla name.

Hey, if we had a PTO app that tracked all absences, we could integrate
with it...


> Some review tools track review comments, so checking subsequent patches
> is easier. A couple months, I heard discussion about an investigation of
> Review Board for Bugzilla. Is anyone still investigating Review Board?

Yes; it's on the BMO roadmap to be integrated; I believe its even being
worked on now. CCing glob.

Gerv
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Gijs Kruitbosch


On 11/07/13 09:08 , Robert O'Callahan wrote:

On Wed, Jul 10, 2013 at 3:26 PM, Justin Lebar wrote:


If I can propose something that's perhaps different:

1) Write software to figure out who's "slow with reviews".
2) We talk to those people.



We've done this before too.

But we should just do it again --- the "definition of insanity" aphorism is
nonsense. Repetition helps people learn.

Rob


I am confused regarding "figure out who's 'slow with reviews'". Taras' 
original proposal explicitly outlined the option of giving an ETA 
because it's not always possible to be "super fast with reviews", to put 
it that way. How do we intend to software-solve this ETA giving rather 
than r+/-/_ setting? Or am I making perfect the enemy of the good here? :-)


~ Gijs
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Gijs Kruitbosch


On 11/07/13 12:09 , Nicholas Nethercote wrote:

On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden  wrote:


Establishing one-day turnaround time on reviews, or on requests, would require 
a lot more polling on my review queue.


You poll your review queue?  Like, by visiting your Bugzilla
dashboard, or something like that?  That's *awful*.

I personally use a push notification system called "email with
filters".  Well, strictly speaking it's poll-like because I have to
check my "high priority bugs" folder, but I do that anyway multiple
times per day so I'm unlikely to take more than an hour or two (while
working) to notice a review request.

Seriously, if your bug management system causes you to not notice
review requests very quickly, it is broken.

Nick




I find this fascinating. I am like Nick in this respect (although I 
don't need to filter out review requests from other bugmail, I do use 
filters for bugmail and will notice them quickly enough, generally 
speaking). My email client is always open and I generally notice bugmail 
more or less as it happens.


However, contemporary wisdom seems to be that people get more done if 
they use pull rather than push systems. I'm not sure we should force 
people to use a push system here (although I do think it'd be good to 
have reviews go faster / get ETAs if they can't go fast).


~ Gijs
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Nicholas Nethercote

On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden  wrote:
>
> Establishing one-day turnaround time on reviews, or on requests, would 
> require a lot more polling on my review queue.

You poll your review queue?  Like, by visiting your Bugzilla
dashboard, or something like that?  That's *awful*.

I personally use a push notification system called "email with
filters".  Well, strictly speaking it's poll-like because I have to
check my "high priority bugs" folder, but I do that anyway multiple
times per day so I'm unlikely to take more than an hour or two (while
working) to notice a review request.

Seriously, if your bug management system causes you to not notice
review requests very quickly, it is broken.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-11 Thread Nicholas Nethercote

On Thu, Jul 11, 2013 at 5:06 PM, Robert O'Callahan  wrote:
> On Wed, Jul 10, 2013 at 6:09 AM, smaug  wrote:
>
>> One thing, which has often brought up, would be to have other automatic
>> coding style checker than just Ms2ger.
>
> See https://bugzilla.mozilla.org/show_bug.cgi?id=875605, which is,
> ironically, waiting for review.

https://bugzilla.mozilla.org/show_bug.cgi?id=880088 is similar (albeit
SpiderMonkey-only), and is also waiting for review!

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

On Wed, Jul 10, 2013 at 2:48 PM, Jeff Walden  wrote:

> Reviewer-side: I've lately tried to minimize my review turnaround time
> such that I get things done, roughly FIFO, before I get a review-nag mail.
>  I can approximately do that, while keeping up with most of my coding.
>  Establishing one-day turnaround time on reviews, or on requests, would
> require a lot more polling on my review queue.  And I don't think that's a
> good thing, context-switching from coding tasks being pretty expensive for
> any developer.
>

I think a reasonable expectation is that paid staff process outstanding
review requests at least once every regular work day. If you do it at the
beginning of the day, before diving into your other work, it won't
interrupt that other work.

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

We can't have a rigid rule about 24 hours. Someone requesting a review from
me on Thursday PDT probably won't get a response until Monday if neither of
us work during the weekend.

But I think it's reasonable to expect developers to process outstanding
review requests (and needinfos) at least once every regular work day.
Processing includes leaving a comment with an ETA.

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

On Wed, Jul 10, 2013 at 3:26 PM, Justin Lebar wrote:

> If I can propose something that's perhaps different:
>
> 1) Write software to figure out who's "slow with reviews".
> 2) We talk to those people.
>

We've done this before too.

But we should just do it again --- the "definition of insanity" aphorism is
nonsense. Repetition helps people learn.

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

On Wed, Jul 10, 2013 at 6:09 AM, smaug  wrote:

> One thing, which has often brought up, would be to have other automatic
> coding style checker than just Ms2ger.
>

See https://bugzilla.mozilla.org/show_bug.cgi?id=875605, which is,
ironically, waiting for review.

Rob
-- 
Jtehsauts  tshaei dS,o n" Wohfy  Mdaon  yhoaus  eanuttehrotraiitny  eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o  Whhei csha iids  teoa
stiheer :p atroa lsyazye,d  'mYaonu,r  "sGients  uapr,e  tfaokreg iyvoeunr,
'm aotr  atnod  sgaoy ,h o'mGee.t"  uTph eann dt hwea lmka'n?  gBoutt  uIp
waanndt  wyeonut  thoo mken.o w  *
*
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Neil


smaug wrote:

One thing, which has often brought up, would be to have other 
automatic coding style checker than just Ms2ger. At least in the DOM 
land we try to follow the coding style rules rather strictly and it 
would ease reviewers work if there was some good tool which does the 
coding style check automatically. 


Or failing that, is this the sort of thing that mentors should be 
checking for before contributors/interns apply for a full review?


--
Warning: May contain traces of nuts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread L. David Baron

On Tuesday 2013-07-09 15:46 -0400, Boris Zbarsky wrote:
> * When requesting a second review on a patch, provide an interdiff
> so that the reviewer can just verify that the changes you made match
> what they asked for.  Bugzilla's interdiff is totally unsuitable for
> this purpose, unfortunately, because it fails so often.

For what it's worth, I've learned to read diffs of diffs, and I
don't consider it that hard a skill to learn.  I read diffs of diffs
for nearly all re-reviews that I do (even ignoring any interdiff in
the bug).

Given one of:
  alias vs="wget -q -O -"
  alias vs="curl -s"
  alias vs="lynx -source"
I just use (in bash):
  diff -u <(vs 'https://bugzilla.mozilla.org/attachment.cgi?id=688611') <(vs 
'https://bugzilla.mozilla.org/attachment.cgi?id=696690') | less -S

Speaking of re-reviews, I wish bugzilla had a way to indicate that,
since I often get re-reviews long enough after the original review
that I've forgotten it's a re-review.  And I prefer to prioritize
re-reviews highest because it helps me get through the reviews
faster, since it will be faster if I still remember what I was
thinking during the first review.

-David

-- 
𝄞   L. David Baron http://dbaron.org/   𝄂
𝄢   Mozilla   http://www.mozilla.org/   𝄂
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Taras Glek




L. David Baron wrote:

[ responding to the two months worth flood of email that just
   resulted from https://bugzilla.mozilla.org/show_bug.cgi?id=891906 ]

On Tuesday 2013-07-09 12:14 -0700, Taras Glek wrote:

a) Realize that reviewing code is more valuable than writing code as
it results in higher overall project activity. If you find you can't
write code anymore due to prioritizing reviews over coding, grow
more reviewers.


Agreed, as long as the reviews are for things that we actually agree
are important.


b) Communicate better. If you are an active contributor, you should
not leave r? patches sitting in your queue without feedback. "I will
review this next week because I'm (busy reviewing ___ this week|away
at conference). I think bugzilla could use some improvements there.
If you think a patch is lower priority than your other work
communicate that.

c) If you think saying nothing is better than admitting than you
wont get to the patch for a while**, that's passive aggressiveness
(https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is
not a good way to build a happy coding community. Managers, look for
instances of this on your team.


I think there's a distinction between review requests:  some of the
review requests I recieve are assertions "I believe this code is
right, could you check?".  Some of them aren't; they're "this seems
to work, but I really have no idea if it's correct; is it?".
I added a low-priority flag request to 
https://wiki.mozilla.org/Code_Review#Bugzilla_Improvements


I agree that we should focus on clear rules for good patches. If a patch 
author doesn't quite know how to accomplish something, that's more of f? 
or a low-priority flag.


I think we should perhaps be able to have an expectation of fast
response on the first set of review requests, but I don't think we
should have an expectation of fast response on the second half,
since many of them require the reviewer to do more work than the
patch author.  (I think I get a pretty substantial number of this
form of review requests, at least when counting percentage of time
rather than percentage of requests.)

But sometimes it's also not clear which category the review request
is in, or sometimes it's somewhere in-between.  (Maybe we should ask
people to distinguish the types?  Should people then be embarrassed
to get a review- on a patch of the first type where they're told to
go back to the drawing board?)


Or maybe I should just summarily minus review requests that appear
to be of the second form, perhaps with pointers as to how the patch
author should learn what's needed to figure out the necessary
information?


I also agree with Boris's comments about things that patch authors
should do to make patches easier to review.  I should probably be
better about using review- when patch authors don't do these things,
though I often feel bad about doing that when I've been away for a
week and spent a few days catching up, and it's a patch that's
already been sitting there for ten days.  I guess I should just do
it anyway.

My list of things would be:

  * make the summary of the bug reflect the problem so that there's a
clear description of what the patch is trying to fix

  * split things into small, logical, patches

  * write good commit messages that describe what's changing between
old and new code (which, if it can't be summarized in less than
about 100-150 characters, should have a short summary on the
first line and a longer description on later lines)

  * write good code comments that describe the state of the new code,
and if the patch is of nontrivial size, point to the important
comments in the non-first lines of the commit message


Would you mind reworking above wiki with your points?


Taras




-David


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread L. David Baron

[ responding to the two months worth flood of email that just
  resulted from https://bugzilla.mozilla.org/show_bug.cgi?id=891906 ]

On Tuesday 2013-07-09 12:14 -0700, Taras Glek wrote:
> a) Realize that reviewing code is more valuable than writing code as
> it results in higher overall project activity. If you find you can't
> write code anymore due to prioritizing reviews over coding, grow
> more reviewers.

Agreed, as long as the reviews are for things that we actually agree
are important.

> b) Communicate better. If you are an active contributor, you should
> not leave r? patches sitting in your queue without feedback. "I will
> review this next week because I'm (busy reviewing ___ this week|away
> at conference). I think bugzilla could use some improvements there.
> If you think a patch is lower priority than your other work
> communicate that.
> 
> c) If you think saying nothing is better than admitting than you
> wont get to the patch for a while**, that's passive aggressiveness
> (https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is
> not a good way to build a happy coding community. Managers, look for
> instances of this on your team.

I think there's a distinction between review requests:  some of the
review requests I recieve are assertions "I believe this code is
right, could you check?".  Some of them aren't; they're "this seems
to work, but I really have no idea if it's correct; is it?".

I think we should perhaps be able to have an expectation of fast
response on the first set of review requests, but I don't think we
should have an expectation of fast response on the second half,
since many of them require the reviewer to do more work than the
patch author.  (I think I get a pretty substantial number of this
form of review requests, at least when counting percentage of time
rather than percentage of requests.)

But sometimes it's also not clear which category the review request
is in, or sometimes it's somewhere in-between.  (Maybe we should ask
people to distinguish the types?  Should people then be embarrassed
to get a review- on a patch of the first type where they're told to
go back to the drawing board?)

Or maybe I should just summarily minus review requests that appear
to be of the second form, perhaps with pointers as to how the patch
author should learn what's needed to figure out the necessary
information?

I also agree with Boris's comments about things that patch authors
should do to make patches easier to review.  I should probably be
better about using review- when patch authors don't do these things,
though I often feel bad about doing that when I've been away for a
week and spent a few days catching up, and it's a patch that's
already been sitting there for ten days.  I guess I should just do
it anyway.

My list of things would be:

 * make the summary of the bug reflect the problem so that there's a
   clear description of what the patch is trying to fix

 * split things into small, logical, patches

 * write good commit messages that describe what's changing between
   old and new code (which, if it can't be summarized in less than
   about 100-150 characters, should have a short summary on the
   first line and a longer description on later lines)

 * write good code comments that describe the state of the new code,
   and if the patch is of nontrivial size, point to the important
   comments in the non-first lines of the commit message

-David

-- 
𝄞   L. David Baron http://dbaron.org/   𝄂
𝄢   Mozilla   http://www.mozilla.org/   𝄂
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Mark Côté

On 2013-07-10 2:18 PM, Boris Zbarsky wrote:
> On 7/10/13 1:58 PM, Mark Côté wrote:
>> The BMO team is again considering switching to ReviewBoard, which should
>> fix this problem
> 
> How does ReviewBoard address this?
> 
> Again, what we have in the bug is diff 1 against changeset A and diff 2
> against changeset B that incorporates the review changes.  What the
> reviewer would like to see is a diff from 1 rebased against B to 2.
> 
> I guess that's possible if the system tries to automatically rebase diff
> 1 against changeset B, but if the automatic rebase fails you still fail...

True, sorry, I meant only the part about it having access to source
control.  Apologies for the diversion.

Mark



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Justin Lebar

One definition of insanity is doing the same thing twice and expecting
different results.

I recall that Taras has written basically this same e-mail before.  We
seem to have this conversation every six months or so.  Why do we
expect different results this time?

If I can propose something that's perhaps different:

1) Write software to figure out who's "slow with reviews".
2) We talk to those people.

This is basically the same approach as we had with the tryserver
highscore board.  It seems to have helped (?).

I think Bugzilla archaeology was intended to help with [1], but that
site is not useful for this purpose, IMO.  But I don't see anything
stopping us from actually writing this software.

[1] https://metrics.mozilla.com/bugzilla-analysis/ReviewHistory.html

On Tue, Jul 9, 2013 at 3:14 PM, Taras Glek  wrote:
> Hi,
> Browsers are a competitive field. We need to move faster. Eliminating review
> lag is an obvious step in the right direction.
>
> I believe good code review is essential for shipping a good browser.
>
> Conversely, poor code review practices hold us back. I am really frustrated
> with how many excellent developers are held back by poor review practices.
> IMHO the single worst practice is not communicating with patch author as to
> when the patch will get reviewed.
>
> Anecdotal evidence suggests that we do best at reviews where the patch in
> question lines up with reviewer's current project The worst thing that
> happens there is rubber-stamping (eg reviewing non-trivial 60KB+ patches in
> 30min).
>
> Anecdotally, latency correlates inversely with how close the reviewer is to
> patch author, eg:
>
> project > same team > same part of organization > org-wide > random
> community member
>
> I think we need change a couple things*:
>
> a) Realize that reviewing code is more valuable than writing code as it
> results in higher overall project activity. If you find you can't write code
> anymore due to prioritizing reviews over coding, grow more reviewers.
>
> b) Communicate better. If you are an active contributor, you should not
> leave r? patches sitting in your queue without feedback. "I will review this
> next week because I'm (busy reviewing ___ this week|away at conference). I
> think bugzilla could use some improvements there. If you think a patch is
> lower priority than your other work communicate that.
>
> c) If you think saying nothing is better than admitting than you wont get to
> the patch for a while**, that's passive aggressiveness
> (https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is not a
> good way to build a happy coding community. Managers, look for instances of
> this on your team.
>
> In my experience the main cause of review stop-energy is lack of will to
> inconvenience own projects by switching gears to go through another person's
> work.
>
> I've seen too many amazing, productive people get discouraged by poor review
> throughput. Most of these people would rather not create even more tension
> by complaining about this...that's what managers are for :)
>
> Does anyone disagree with my 3 points above? Can we make some derivative of
> these rules into a formal policy(some sort of code of developer conduct)?
>
> Taras
>
> * There obvious exceptions to above guidelines (eg deadlines).
> ** Holding back bad code is a feature, not a bug, do it politely.
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Taras Glek




Boris Zbarsky wrote:

On 7/9/13 9:59 PM, therealbrendane...@gmail.com wrote:

Yes, that's what I meant. How else could one respond within a day?


Some of the "within a day" proposals have suggested that it include
weekends, fwiw.


Ok. Does this need to go on wiki.m.o or MDN somewhere (not that it
would help those who don't read the updated pages)? Should I use a
bigger bully pulpit?


Having something on wikimo for both requester and requestee best
practices would be a good start.


I tried to capture feedback from this thread in 
https://wiki.mozilla.org/Code_Review



Taras
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Jeff Walden

On 07/09/2013 07:17 PM, Joshua Cranmer 🐧 wrote:
>> I've said this before, not sure it's written in wiki-stone, maybe it should 
>> be: if you get a review request, respond same-day either with the actual 
>> review, or an ETA or promise to review by a certain date.
> 
> For reviewers who are not Mozilla employees but rather do this only in spare 
> time, responding within 24 hours can be difficult. I typically end up doing 
> most reviews in a weekly-ish batch on the weekends, for example.

I find myself wondering: why 24h, exactly?  Why is a day the limit on the time 
to wait?

Reviewer-side: I've lately tried to minimize my review turnaround time such 
that I get things done, roughly FIFO, before I get a review-nag mail.  I can 
approximately do that, while keeping up with most of my coding.  Establishing 
one-day turnaround time on reviews, or on requests, would require a lot more 
polling on my review queue.  And I don't think that's a good thing, 
context-switching from coding tasks being pretty expensive for any developer.

Reviewee-side: maybe I'm just weird, or maybe JS is just different, but I 
always have half a dozen to a dozen different things I'm doing, or could be 
doing, at any time.  If I find myself waiting for forward progress one one -- 
waiting for try results, waiting for a review, waiting for my clock to overlap 
with that of someone in Europe, or anything else -- I can work on something 
else, and not feel obligated to interrupt whoever/whatever I'm waiting on to 
ask for prioritization.  Outside of must-fix-for-security-uplift-this-week 
situations, I can't think of a case where 24h turnaround (even to provide 
expectations) is something I've ever wanted so strongly that I'd force every 
reviewer to drop enough things to provide it.

Or maybe I'm just weird.  But 24h, for much other than new-reviewer requests, 
seems way shorter than I think the maximum turnaround time should be.  If it's 
a new contributor, okay, 24h or so (modulo weekends, vacations, etc.) is a bit 
short, but perhaps doable.  But for regular contributors, with many things to 
do while waiting for any particular review, I don't see why the delay shouldn't 
be a few days, to a week, to maybe slightly over a week.

Jeff
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread msreckovic

On Wednesday, 10 July 2013 13:06:04 UTC-4, Boris Zbarsky  wrote:
> On 7/10/13 12:56 PM, Milan wrote:
> 
> > Why not?
> 
> 
> 
> Because submitting a first patch is scary enough as it is that we should 
> 
> try to minimize the roadblocks involved in it.
> 
> 
> 
> This is also why the reviewer in cases like that should handle setting 
> 
> the checkin-needed keyword (or just land the patch), push it to try as 
> 
> needed, etc.
> 
> 
> 
> > If there is a list of good patch practices, there is no reason we can't ask 
> > people to complete a checklist and comment on it.
> 
> 
> 
> I think that's a fine thing to do for people who are planning to do more 
> 
> than one patch, and is definitely something we should do when someone 
> 
> starts contributing somewhat consistently.  But I'm not convinced that 
> 
> it's reasonable to require it for a first patch...
> 
> 
> 
> -Boris

Fair enough.  We don't want to scare people off.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/10/13 8:31 AM, Mark Banner wrote:

The problem is, that doesn't work on the patch submission forms.


Or on bzexport.  I can't recall the last time I used the Bugzilla UI for 
requesting review...



but I think it would be good
to have the option to provide a warning with a little bit of text *and*
have that a) displayed on the submission forms, and b)
prompted/confirmed if the user decides to request anyway.


And communicated via bzapi so bzexport can also warn.

-Boris

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Joshua Cranmer 🐧


On 7/9/2013 5:11 PM, therealbrendane...@gmail.com wrote:

Good news is bugzilla is getting attention now, both back-end and front-end. 
More on that separately, because it's not the main point of Taras's post.

The main point is that review is mandatory and must be prompt or the whole peer 
review potlatch system breaks down.

I've said this before, not sure it's written in wiki-stone, maybe it should be: 
if you get a review request, respond same-day either with the actual review, or 
an ETA or promise to review by a certain date.


For reviewers who are not Mozilla employees but rather do this only in 
spare time, responding within 24 hours can be difficult. I typically end 
up doing most reviews in a weekly-ish batch on the weekends, for example.


--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread therealbrendaneich

Good news is bugzilla is getting attention now, both back-end and front-end. 
More on that separately, because it's not the main point of Taras's post.

The main point is that review is mandatory and must be prompt or the whole peer 
review potlatch system breaks down.

I've said this before, not sure it's written in wiki-stone, maybe it should be: 
if you get a review request, respond same-day either with the actual review, or 
an ETA or promise to review by a certain date.

People may need reminding or nagging but that should be the exception. It 
sounds like it isn't exceptional, or rare enough.

/be
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Chris Peterson


I really
wish Bugzilla could let me flag myself as not available for reviews when
I'm on vacation, say.  Expecting people to comment about being on
vacation while on vacation is, imo, not reasonable.


I've seen people change their Bugzilla name to include a comment about 
being on PTO. We should promote this practice. We could also add a 
Bugzilla feature (just a simple check box or a PTO date range) that 
appends some vacation message to your Bugzilla name.




* Actually address the review comments before requesting another review.
  It's very common to just ignore (miss?) some of the review comments,
which means the reviewer then needs to triple-check that all the things
they pointed out got fixed.


Some review tools track review comments, so checking subsequent patches 
is easier. A couple months, I heard discussion about an investigation of 
Review Board for Bugzilla. Is anyone still investigating Review Board?




Bugzilla's interdiff is totally unsuitable for this
purpose, unfortunately, because it fails so often.


Can we fix Bugzilla's interdiff?


chris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

review stop-energy (was 24hour review)

2013-07-10 Thread Taras Glek


Hi,
Browsers are a competitive field. We need to move faster. Eliminating 
review lag is an obvious step in the right direction.


I believe good code review is essential for shipping a good browser.

Conversely, poor code review practices hold us back. I am really 
frustrated with how many excellent developers are held back by poor 
review practices. IMHO the single worst practice is not communicating 
with patch author as to when the patch will get reviewed.


Anecdotal evidence suggests that we do best at reviews where the patch 
in question lines up with reviewer's current project The worst thing 
that happens there is rubber-stamping (eg reviewing non-trivial 60KB+ 
patches in 30min).


Anecdotally, latency correlates inversely with how close the reviewer is 
to patch author, eg:


project > same team > same part of organization > org-wide > random 
community member


I think we need change a couple things*:

a) Realize that reviewing code is more valuable than writing code as it 
results in higher overall project activity. If you find you can't write 
code anymore due to prioritizing reviews over coding, grow more reviewers.


b) Communicate better. If you are an active contributor, you should not 
leave r? patches sitting in your queue without feedback. "I will review 
this next week because I'm (busy reviewing ___ this week|away at 
conference). I think bugzilla could use some improvements there. If you 
think a patch is lower priority than your other work communicate that.


c) If you think saying nothing is better than admitting than you wont 
get to the patch for a while**, that's passive aggressiveness 
(https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is not 
a good way to build a happy coding community. Managers, look for 
instances of this on your team.


In my experience the main cause of review stop-energy is lack of will to 
inconvenience own projects by switching gears to go through another 
person's work.


I've seen too many amazing, productive people get discouraged by poor 
review throughput. Most of these people would rather not create even 
more tension by complaining about this...that's what managers are for :)


Does anyone disagree with my 3 points above? Can we make some derivative 
of these rules into a formal policy(some sort of code of developer conduct)?


Taras

* There obvious exceptions to above guidelines (eg deadlines).
** Holding back bad code is a feature, not a bug, do it politely.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/10/13 1:58 PM, Mark Côté wrote:

The BMO team is again considering switching to ReviewBoard, which should
fix this problem


How does ReviewBoard address this?

Again, what we have in the bug is diff 1 against changeset A and diff 2 
against changeset B that incorporates the review changes.  What the 
reviewer would like to see is a diff from 1 rebased against B to 2.


I guess that's possible if the system tries to automatically rebase diff 
1 against changeset B, but if the automatic rebase fails you still fail...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/10/13 12:56 PM, msrecko...@mozilla.com wrote:

Why not?


Because submitting a first patch is scary enough as it is that we should 
try to minimize the roadblocks involved in it.


This is also why the reviewer in cases like that should handle setting 
the checkin-needed keyword (or just land the patch), push it to try as 
needed, etc.



If there is a list of good patch practices, there is no reason we can't ask 
people to complete a checklist and comment on it.


I think that's a fine thing to do for people who are planning to do more 
than one patch, and is definitely something we should do when someone 
starts contributing somewhat consistently.  But I'm not convinced that 
it's reasonable to require it for a first patch...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread smaug

On 07/09/2013 03:14 PM, Taras Glek wrote:

Hi,
Browsers are a competitive field. We need to move faster. Eliminating review
lag is an obvious step in the right direction.

I believe good code review is essential for shipping a good browser.

Conversely, poor code review practices hold us back. I am really frustrated
with how many excellent developers are held back by poor review practices.
IMHO the single worst practice is not communicating with patch author as to
when the patch will get reviewed.

Anecdotal evidence suggests that we do best at reviews where the patch in
question lines up with reviewer's current project The worst thing that
happens there is rubber-stamping (eg reviewing non-trivial 60KB+ patches in
30min).

Anecdotally, latency correlates inversely with how close the reviewer is to
patch author, eg:

project > same team > same part of organization > org-wide > random community
member

I think we need change a couple things*:

a) Realize that reviewing code is more valuable than writing code as it results
in higher overall project activity. If you find you can't write code
anymore due to prioritizing reviews over coding, grow more reviewers.

b) Communicate better. If you are an active contributor, you should not leave r?
patches sitting in your queue without feedback. "I will review this
next week because I'm (busy reviewing ___ this week|away at conference). I
think bugzilla could use some improvements there. If you think a patch is
lower priority than your other work communicate that.

c) If you think saying nothing is better than admitting than you wont get to
the patch for a while**, that's passive aggressiveness
(https://en.wikipedia.org/wiki/Passive-aggressive_behavior). This is not a good
way to build a happy coding community. Managers, look for instances of
this on your team.

In my experience the main cause of review stop-energy is lack of will to
inconvenience own projects by switching gears to go through another person's
work.

I've seen too many amazing, productive people get discouraged by poor review
throughput. Most of these people would rather not create even more
tension by complaining about this...that's what managers are for :)

Does anyone disagree with my 3 points above? Can we make some derivative of
these rules into a formal policy(some sort of code of developer conduct)?

Taras

* There obvious exceptions to above guidelines (eg deadlines).
** Holding back bad code is a feature, not a bug, do it politely.

In general, +1 to all the 3 points. For b) it would be nice if bugzilla would let also the patch author to indicate if some patch isn't anything
urgent. (or perhaps the last sentence of b) means that. Not sure whether 'you' refers to the reviewer or the patch author :) )

One thing, which has often brought up, would be to have other automatic coding
style checker than just Ms2ger.
At least in the DOM land we try to follow the coding style rules rather
strictly and it would ease reviewers work if
there was some good tool which does the coding style check automatically.

Curious, do we have some recent statistics how long it takes to get a review?
Hopefully per module.

On 07/09/2013 03:46 PM, Boris Zbarsky wrote:

> * Split mass-changes or mechanical changes into a separate patch from the
substantive changes.
>
> * If possible, separate patches into conceptually-separate pieces for review
purposes (even if you then later collapse them into a single changeset to
> push). Any time you're requesting review from multiple people on a single
huge diff, chance are splitting it might have been a good idea.
...

Splitting patches is usually useful, but having a patch containing all the
changes can be also good.
If you have a set of 20-30 patches, but not a patch which contains all the
changes, it is often hard to see the big picture.
Again, perhaps some tool could help here. Something which can generate the full
patch from the smaller ones.

-Olli
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Anthony Ricaud


On 10/07/13 15:09, smaug wrote:

One thing, which has often brought up, would be to have other automatic
coding style checker than just Ms2ger.
At least in the DOM land we try to follow the coding style rules rather
strictly and it would ease reviewers work if
there was some good tool which does the coding style check automatically.
In Gaia, we have a Git pre-commit hook that runs our linter for every 
commit (if the committer has installed the linter).


You can also see that we only run it on specific directories.

(And in case you know what you're doing, you can bypass it with |git 
commit --no-verify| )


https://github.com/mozilla-b2g/gaia/blob/master/tools/pre-commit
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/9/13 6:11 PM, therealbrendane...@gmail.com wrote:

I've said this before, not sure it's written in wiki-stone, maybe it should be: 
if you get a review request, respond same-day either with the actual review, or 
an ETA or promise to review by a certain date.


Again, this is not viable during vacations/weekends...  Unless by "get" 
you mean "get the mail" as opposed to "have it appear in your queue".



People may need reminding or nagging but that should be the exception. It 
sounds like it isn't exceptional, or rare enough.


Indeed.

-Boris

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/9/13 4:29 PM, Chris Peterson wrote:

I've seen people change their Bugzilla name to include a comment about
being on PTO.


Sure.  As a simple example, I did that on June 20th.  I got about 20 
review requests over the course of the following week and a half, and 
that's with most of the people who would normally ask me for review 
_not_ doing so because they knew I was away.



Bugzilla's interdiff is totally unsuitable for this
purpose, unfortunately, because it fails so often.


Can we fix Bugzilla's interdiff?


Not easily, because it does not have access to the original code...  The 
most common failure mode here is something like this:


1)  Author posts patch.
2)  Review happens.
3)  Author rebases patch to tip, makes edits to address review
comments, re-requests review.

At this point, bugzilla interdiff between the new patch and the old one 
will only work if the rebase was exceedingly trivial.  Further, what's 
really desired in most cases is the diff between the rebased patch and 
the patch that addresses comments, not the diff between the old patch 
and the new patch.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Mark Côté

On 2013-07-09 4:48 PM, Boris Zbarsky wrote:> On 7/9/13 4:29 PM, Chris
Peterson wrote:
>>> Bugzilla's interdiff is totally unsuitable for this
>>> purpose, unfortunately, because it fails so often.
>>
>> Can we fix Bugzilla's interdiff?
>
> Not easily, because it does not have access to the original code...

The BMO team is again considering switching to ReviewBoard, which should
fix this problem, at least for our most-used repos.  When we last
evaluated this option, a few years ago, there actually wasn't a BMO
team, so we went with the easier option (Splinter), since it is
nontrivial to do proper Bugzilla-ReviewBoard integration.  We have more
resources now, and ReviewBoard seems to answer a lot of user complaints
about code reviews, so we're working on this anew.

Mark

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread Mark Banner

On 09/07/2013 21:29, Chris Peterson wrote:
>> I really
>> wish Bugzilla could let me flag myself as not available for reviews when
>> I'm on vacation, say.  Expecting people to comment about being on
>> vacation while on vacation is, imo, not reasonable.
> 
> I've seen people change their Bugzilla name to include a comment about
> being on PTO. We should promote this practice. We could also add a
> Bugzilla feature (just a simple check box or a PTO date range) that
> appends some vacation message to your Bugzilla name.

The problem is, that doesn't work on the patch submission forms. If
bugzilla does decide to autocomplete, you can't necessarily see all the
info on the name, because the field isn't wide enough.

If you don't need/look at the autocomplete, then you get zero indication
of the  bugzilla name.

The only real chance you get to see that name change is if you're on a
bug where they have already commented or filed.

I wouldn't want something just for PTOs (e.g. meetups can take up review
time for projects I'm not meeting up for), but I think it would be good
to have the option to provide a warning with a little bit of text *and*
have that a) displayed on the submission forms, and b)
prompted/confirmed if the user decides to request anyway.

Mark.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread msreckovic

On Tuesday, 9 July 2013 15:46:31 UTC-4, Boris Zbarsky  wrote:
> On 7/9/13 3:14 PM, Taras Glek wrote:
> 
> > Conversely, poor code review practices hold us back.
> 
> 
> 
> Agreed.  At the same time, poor patch practices make reviews _much_ 
> 
> harder.  We should generally expect good patch practices from 
> 
> established contributors; obviously expecting them from new contributors 
> 
> is not reasonable.
> 

Why not?  If there is a list of good patch practices, there is no reason we 
can't ask people to complete a checklist and comment on it. It would probably 
also let us train more reviewers, but letting them know what to start with 
checking.

> 
> 
> I believe that getting fast review turnaround is a collaboration between 
> 
> the reviewer and review requester; it shouldn't be solely on the 
> 
> reviewer to do a fast review no matter how hard the requester makes it. 

+1. "It's so easy to review your code" should be one of the highest praises for 
a developer (code quality being equal).

--
- Milan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)

2013-07-10 Thread therealbrendaneich

On Tuesday, July 9, 2013 6:49:20 PM UTC-7, Boris Zbarsky wrote:
> On 7/9/13 6:11 PM, brendan wrote:
> 
> > I've said this before, not sure it's written in wiki-stone, maybe it should 
> > be: if you get a review request, respond same-day either with the actual 
> > review, or an ETA or promise to review by a certain date.
> 
> 
> 
> Again, this is not viable during vacations/weekends...  Unless by "get" 
> 
> you mean "get the mail" as opposed to "have it appear in your queue".

Yes, that's what I meant. How else could one respond within a day?

> > People may need reminding or nagging but that should be the exception. It 
> > sounds like it isn't exceptional, or rare enough.
> 
> 
> 
> Indeed.


Ok. Does this need to go on wiki.m.o or MDN somewhere (not that it would help 
those who don't read the updated pages)? Should I use a bigger bully pulpit?

/be
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)


On 7/9/13 9:59 PM, therealbrendane...@gmail.com wrote:

Yes, that's what I meant. How else could one respond within a day?


Some of the "within a day" proposals have suggested that it include 
weekends, fwiw.



Ok. Does this need to go on wiki.m.o or MDN somewhere (not that it would help 
those who don't read the updated pages)? Should I use a bigger bully pulpit?


Having something on wikimo for both requester and requestee best 
practices would be a good start.


-Boris

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: review stop-energy (was 24hour review)