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.
I added a Tips and Tricks section, with some tips on practices to make
interdiff creation easier.
--
Randell
On 11/07/13 14:24, Boris Zbarsky wrote:
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...
sigh
Just in case you were talking about the moco PTO app, it doesn't track
absences for non-MoCo employees, and even for
On 7/10/2013 3:09 PM, smaug wrote:
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
On 7/15/13 7:10 AM, Honza Bambas wrote:
- providing patch split to logically separated parts with numbers like
part 1 of 6
- and also a complete (folded) patch for reference
- strictly versioning the patch among review rounds
- when submitting a new version of a patch after r- always explain
On Mon 15 Jul 2013 11:43:05 AM PDT, Boris Zbarsky wrote:
On 7/15/13 2:36 PM, Chris Peterson wrote:
If reviewee submits a new version of (say) patch 1 of 6, should they:
* attach patch 1 version 2
* an interdiff between patch 1 version 1 and 2
Yes, to both.
Bleagh. All of this points to an
On 7/15/13 2:58 PM, Steve Fink wrote:
Even for the case of dependent patches (ones with separate parts that
cannot be landed together, but are usefully split up for review)?
Assuming s/cannot/must/, that's why I said generally, not always. ;)
Perhaps that's wrong of me, but it seems like
On 7/15/2013 2:58 PM, Steve Fink wrote:
On Mon 15 Jul 2013 11:43:05 AM PDT, Boris Zbarsky wrote:
On 7/15/13 2:36 PM, Chris Peterson wrote:
If reviewee submits a new version of (say) patch 1 of 6, should they:
* attach patch 1 version 2
* an interdiff between patch 1 version 1 and 2
Yes, to
On 11/07/13 16:43, Neil wrote:
Milan Sreckovic wrote:
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.
Even better would be if Bugzilla could compute
On 2013-07-12, at 11:46 , Mounir Lamouri mou...@lamouri.fr wrote:
On 11/07/13 16:43, Neil wrote:
Milan Sreckovic wrote:
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
On Wed, Jul 10, 2013 at 3:26 PM, Justin Lebar justin.le...@gmail.comwrote:
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
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
On Wed, Jul 10, 2013 at 2:48 PM, Jeff Walden jwalden+...@mit.edu 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.
On Thu, Jul 11, 2013 at 5:06 PM, Robert O'Callahan rob...@ocallahan.org wrote:
On Wed, Jul 10, 2013 at 6:09 AM, smaug sm...@welho.com wrote:
One thing, which has often brought up, would be to have other automatic
coding style checker than just Ms2ger.
See
On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden jwalden+...@mit.edu 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
On 11/07/13 12:09 , Nicholas Nethercote wrote:
On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden jwalden+...@mit.edu 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
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
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
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
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...
sigh
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
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
On 2013-07-11 3:06 AM, Robert O'Callahan wrote:
On Wed, Jul 10, 2013 at 6:09 AM, smaug sm...@welho.com 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,
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
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.
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
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
On Thu, Jul 11, 2013 at 6:23 AM, Justin Lebar justin.le...@gmail.comwrote:
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
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
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
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
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
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
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
On 07/11/2013 03:09 AM, Nicholas Nethercote wrote:
On Thu, Jul 11, 2013 at 7:48 AM, Jeff Walden jwalden+...@mit.edu 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
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 jwalden+...@mit.edu wrote:
Establishing one-day turnaround time on reviews, or on requests, would require
a lot more polling on my review queue.
You poll
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
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
Milan Sreckovic wrote:
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.
Even better would be if Bugzilla could compute their median review
turnaround for
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
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
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
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
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...
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
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
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
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
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.
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
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
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
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
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
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
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
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
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
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
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
[ 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
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
60 matches
Mail list logo
