>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.
--
R
On 7/15/2013 8:36 PM, Chris Peterson wrote:
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
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 b
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 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 a
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.
* and a new complete/folded patch (of patches 1-6)?
Usually not needed.
Which file would
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 wh
On 2013-07-11 2:14 PM, Jet Villegas wrote:
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
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 help
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...
>>
>
> Just in case you were talking about the moco PTO app, it doesn't track
> absences for non-MoCo employees, and even fo
On 10/07/13 10:17 AM, Anthony Ricaud wrote:
> 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
On 2013-07-12, at 11:46 , Mounir Lamouri 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 their r
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
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/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.
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? L
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 revie
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
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
conversa
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 revie
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)
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 developer
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
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 eanut
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 "ha
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 c
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 again
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 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 do
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
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) th
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 (bein
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 th
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
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.or
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 Bugzil
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
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
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 pers
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,
>
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-da
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 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" a
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"
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
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, b
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 hi
[ 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 act
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
> a
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
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
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 Mo
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 c
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 tex
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
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 s
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
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 land
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 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 t
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 durin
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 peop
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
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
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 practi
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 r
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
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
69 matches
Mail list logo
