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
