Rob Lanphier wrote:
On Tue, May 31, 2011 at 10:35 AM, Neil Kandalgaonkar
<neilk(a)wikimedia.org> wrote:
> Are we all in deadlock or something?
<snip>
independent suggestions, but any of these might help:
1. We say that a commit has some fixed window (e.g. 72 hours) to get
reviewed, or else it is subject to automatic reversion. This will
motivate committers to make sure they have a reviewer lined up, and
make it clear that, if their code gets reverted, it's nothing
personal...it's just our process.
2. We encourage committers to identify who will be reviewing their
code as part of their commit comment. That way, we have an identified
person who has license to revert if they don't understand the code.
Russell N. Nelson - rnnelson wrote:
Worse than pre-commit review. What if you make a
change, I see it,
and make changes to my code using your changes. 72 hours later, they
get reverted, which screws my code. Okay, so then nobody's going to
compile anything, or use anything, if it has 72-hour code in it. The
alternative, if they want the code badly enough, is to review it so
it will stick. Well, that devolves to the same thing as pre-commit
review.
And ... this only makes sense for core, or maybe for stable
extensions. It doesn't make sense for experimental extensions where
only one person is likely to understand or care what the code says.
Perhaps a combination is possible, a bit like Brion did in the old days:
* A revision has to be reviewed within reasonble time (time upto it
still allows reverting without pain, ie. hours or days. Not weeks! No
more than, say, 3 days)
* The people who do code review, which should always start at the
back, will see soon enough if there's a revision stuck in the process
(ie. not being reviewed / passed to someone else) at that point
someone has to go bold and revert it. Nothing personal: If nobody
understood your commit, either splits it up and document it better –
or poke whoever is capable of reviewing your code and make sure he or
she does so in time. We are responsible as a team (nothign like
primary school hand-in-hand pairs of buddies, but you get the idea..).
* A fixme should not maintain it's status for more than 24 hours. Not
fixed within 24 hours ? Revert, no questions asked!
* Once a week, on Monday, we make sure anything from after Friday noon
(remember, 3 days!) is unreviewed or fixme'ed. They shall be reverted
or sorted out the same day (some gnomes may want to do this over the
weekend).
* Tuesday anything last week should no longer be on our minds. We've
moved on! "Tomorrow" (wednesday) is already the 3rd day of the week.
Aside from the in-svn proccess, some points I learned from other
committers and just now via IRC:
* Unless the committer is subscribed to mediawiki-cvs or keeps a tight
eye on Special:Code, one should have wiki-account linked in the
CodeReview extension and e-mail notifications on (so you know about
questions/comments., status changed and follow-ups as soon as possible)
* the "todo" tag (or soon-to-be-introduced status "needsmore" or
"incomplete")[1] does not mean "in another life time".
* If you don't know where to start...
http://etherpad.wikimedia.org/CodeReviewTracker
--
Krinkle
[1]
https://bugzilla.wikimedia.org/28274