Hello,
I noticed code review lagging a bit in trunk, which I was expecting since 1.17 have kept guru busy for the last month. When I started code reviewing in October or November 2010, I have spent hours and hours reviewing trivial revisions that mostly anyone could have marked ok.
In attempt to get more people to review the code, I have started marking my own trivial commits with the tag 'easy'. It would be a good way to start being confident in reviewing code if you have not yet tried :)
The list is at: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/tag/easy
It would be great if other developers can tag their commits with the same tags and we could start shy developers in reviewing more.
cheers,
On Mon, Feb 28, 2011 at 4:08 PM, Ashar Voultoiz hashar+wmf@free.fr wrote:
I noticed code review lagging a bit in trunk, which I was expecting since 1.17 have kept guru busy for the last month. When I started code reviewing in October or November 2010, I have spent hours and hours reviewing trivial revisions that mostly anyone could have marked ok.
Yeah, 1.17's kept a lot of us busy. Hopefully as the dust on the cluster begins to settle and the regressions are fixed we can get back into high gear for reviewing.
It's true while reviewing you notice a lot of trivial revisions like comment fixes, whitespace cleanup, spelling fixes and the like. The real answer to that is making CR happen often, so those are reviewed and cleaned out regularly.
In attempt to get more people to review the code, I have started marking my own trivial commits with the tag 'easy'. It would be a good way to start being confident in reviewing code if you have not yet tried :)
I really don't agree with this idea. Setting "ok" on a revision implies that the code is ok to run in production environments. While a comment change doesn't really matter, I don't like setting that precedence. In fact, this was the exact reason the "Sign-offs" feature was developed: to encourage people who are not reviewers to say "I've looked at this and it seems ok" or "I've tested this and confirmed that it works." That's actually a lot more useful.
-Chad
"Platonides" Platonides@gmail.com wrote in message news:ikh9dc$9cs$1@dough.gmane.org...
With no pressing timelines, we are slacking off again.
Then let's get a new deadline in place. What's holding us back from timetabling a 1.17wmf2?? It strikes me that the features that will appear in that are fundamentally different from the blockers on the 1.17.0 tarball, and as you say, all the focus seems to be on that at the expense of attention to bleeding-edge code. 1.17 is feature-frozen now, we know what's going into it and what work needs to be done, and we have an implicit deadline (ie ASAP) for getting it finished. A new WMF release deadline in early- to mid-March would give us a much-needed incentive to wage war once again against the CR backlog, and move further towards the continuous-release-to-wmf model that I think we're pretty much unanimously in support of.
--HM
Happy-melon wrote:
Then let's get a new deadline in place. What's holding us back from timetabling a 1.17wmf2?? It strikes me that the features that will appear in that are fundamentally different from the blockers on the 1.17.0 tarball, and as you say, all the focus seems to be on that at the expense of attention to bleeding-edge code. 1.17 is feature-frozen now, we know what's going into it and what work needs to be done, and we have an implicit deadline (ie ASAP) for getting it finished. A new WMF release deadline in early- to mid-March would give us a much-needed incentive to wage war once again against the CR backlog, and move further towards the continuous-release-to-wmf model that I think we're pretty much unanimously in support of.
--HM
Seems a good idea.
On 2/28/11 1:08 PM, Ashar Voultoiz wrote:
It would be great if other developers can tag their commits with the same tags and we could start shy developers in reviewing more.
In general I don't think developers should be deciding that their own commits are easy or hard to review. One line changes can sometimes take forever to verify.
To me this feels like the wrong solution to this problem. People who review code shouldn't be afraid of commits.
This might be an unworkable solution for Wikimedia at present, but I find pre-commit code review tends to align everyone's goals better.
With post-commit review, the developer can commit big chunks of difficult-to-understand code, and the pain of reviewing it is all on the reviewer, and people procrastinate this.
But if the developer can't even commit without a review, like magic, they find ways to break up their changes into small, easy-to-understand, well-documented changes.
I'm a huge fan of pre-commit review. I started to investigate this months ago, before I really understood our Code Review tool. It seemed like it would be too difficult to integrate pre-commit review with our current toolchain.
However, if we ever move to git, maybe we should also think about pre-commit review. (In the git world, that would mean review before merging with the production repo, but basically the same idea).
wikitech-l@lists.wikimedia.org