Rob Lanphier wrote:
On Tue, May 31, 2011 at 10:35 AM, Neil Kandalgaonkar neilk@wikimedia.org wrote:
Are we all in deadlock or something?
<snip>
independent suggestions, but any of these might help:
- 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