Auto-deferral regexes for CodeReview were implemented in r63277 [1], and I deployed this feature three weeks ago. It allows us to set an array of regexes that will be matched against the path of each new commit; if one of them matches, the commit is automatically marked as 'deferred' instead of 'new'.
There are a few limitations to this implementation that are important to understand: * it only matches paths, not commit summaries. This means auto-deferring e.g. TranslateWiki exports is harder * it only matches the root path of the commit, which is very often an uninformative one like /trunk/phase3 , /trunk/extensions , /trunk or even / . This means you can't e.g. auto-defer all commits touching a certain file or path
Despite these limitations, this feature could be quite useful for autodeferring at least some large parts of the repository we don't care about review-wise. Any suggestions for paths to auto-defer?
Roan Kattouw (Catrope)
[1] http://www.mediawiki.org/wiki/Special:Code/MediaWiki/63277
Roan Kattouw schrieb:
Despite these limitations, this feature could be quite useful for autodeferring at least some large parts of the repository we don't care about review-wise. Any suggestions for paths to auto-defer?
http://svn.wikimedia.org/svnroot/mediawiki/trunk/WikiWord/ is basically my personal pet project. If you can keep it out of the review queue, that would probably be an improvement for everyone :)
-- daniel
On Tue, Jul 20, 2010 at 7:46 AM, Daniel Kinzler daniel@brightbyte.de wrote:
Roan Kattouw schrieb:
Despite these limitations, this feature could be quite useful for autodeferring at least some large parts of the repository we don't care about review-wise. Any suggestions for paths to auto-defer?
http://svn.wikimedia.org/svnroot/mediawiki/trunk/WikiWord/ is basically my personal pet project. If you can keep it out of the review queue, that would probably be an improvement for everyone :)
-- daniel
I was going to suggest that. And might I be so bold as to suggest the Semantic* extensions. They're generally marked as deferred without review.
I also want to make a quick disclaimer. This doesn't mean that these code paths don't matter. It just means they aren't actively reviewed on CR, so we want to just automate what reviewers are doing manually anyway
-Chad
Hey,
About the semantic extensions:
It would actually be nice if they did not get marked deferred at all, and be reviewed by people that are familiar with them to some extend. I'm willing to do that for all commits not made by myself. Assuming this would not interfere to much with WMF code review of course :)
Cheers
-- Jeroen De Dauw * http://blog.bn2vs.com * http://wiki.bn2vs.com Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
On 20 July 2010 15:10, Chad innocentkiller@gmail.com wrote:
On Tue, Jul 20, 2010 at 7:46 AM, Daniel Kinzler daniel@brightbyte.de wrote:
Roan Kattouw schrieb:
Despite these limitations, this feature could be quite useful for autodeferring at least some large parts of the repository we don't care about review-wise. Any suggestions for paths to auto-defer?
http://svn.wikimedia.org/svnroot/mediawiki/trunk/WikiWord/ is basically
my
personal pet project. If you can keep it out of the review queue, that
would
probably be an improvement for everyone :)
-- daniel
I was going to suggest that. And might I be so bold as to suggest the Semantic* extensions. They're generally marked as deferred without review.
I also want to make a quick disclaimer. This doesn't mean that these code paths don't matter. It just means they aren't actively reviewed on CR, so we want to just automate what reviewers are doing manually anyway
-Chad
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
About the semantic extensions:
It would actually be nice if they did not get marked deferred at all, and be reviewed by people that are familiar with them to some extend. I'm willing to do that for all commits not made by myself. Assuming this would not interfere to much with WMF code review of course :)
Cheers
If someone's going to start doing code review, that's fine. They've just all been getting deferred because nobody's been reviewing them so far.
-Chad
On 20.07.2010, 17:20 Chad wrote:
On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
About the semantic extensions:
It would actually be nice if they did not get marked deferred at all, and be reviewed by people that are familiar with them to some extend. I'm willing to do that for all commits not made by myself. Assuming this would not interfere to much with WMF code review of course :)
Cheers
If someone's going to start doing code review, that's fine. They've just all been getting deferred because nobody's been reviewing them so far.
We could create a separate review queue for it.
Hey,
That would be completely awesome!
Cheers
-- Jeroen De Dauw * http://blog.bn2vs.com * http://wiki.bn2vs.com Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
On 20 July 2010 17:20, Max Semenik maxsem.wiki@gmail.com wrote:
On 20.07.2010, 17:20 Chad wrote:
On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw jeroendedauw@gmail.com
wrote:
Hey,
About the semantic extensions:
It would actually be nice if they did not get marked deferred at all,
and be
reviewed by people that are familiar with them to some extend. I'm
willing
to do that for all commits not made by myself. Assuming this would not interfere to much with WMF code review of course :)
Cheers
If someone's going to start doing code review, that's fine. They've just all been getting deferred because nobody's been reviewing them so far.
We could create a separate review queue for it.
-- Best regards, Max Semenik ([[User:MaxSem]])
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 20/07/2010 16:25, Jeroen De Dauw wrote:
Hey,
That would be completely awesome!
Yes, that would help a lot, although it might be appropriate to still auto-defer reviews for some extensions in this queue, depending on whether or not enough reviewing happens for them (calling an extension "Semantic..." does not imply that SMW developers will review it ;-).
Markus
Cheers
-- Jeroen De Dauw
Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
On 20 July 2010 17:20, Max Semenikmaxsem.wiki@gmail.com wrote:
On 20.07.2010, 17:20 Chad wrote:
On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauwjeroendedauw@gmail.com
wrote:
Hey,
About the semantic extensions:
It would actually be nice if they did not get marked deferred at all,
and be
reviewed by people that are familiar with them to some extend. I'm
willing
to do that for all commits not made by myself. Assuming this would not interfere to much with WMF code review of course :)
Cheers
If someone's going to start doing code review, that's fine. They've just all been getting deferred because nobody's been reviewing them so far.
We could create a separate review queue for it.
-- Best regards, Max Semenik ([[User:MaxSem]])
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hey,
Is someone planning on doing this? If not, who can do it? The sooner it's there, the better.
Cheers
-- Jeroen De Dauw * http://blog.bn2vs.com * http://wiki.bn2vs.com Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
On 20 July 2010 17:20, Max Semenik maxsem.wiki@gmail.com wrote:
On 20.07.2010, 17:20 Chad wrote:
On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw jeroendedauw@gmail.com
wrote:
Hey,
About the semantic extensions:
It would actually be nice if they did not get marked deferred at all,
and be
reviewed by people that are familiar with them to some extend. I'm
willing
to do that for all commits not made by myself. Assuming this would not interfere to much with WMF code review of course :)
Cheers
If someone's going to start doing code review, that's fine. They've just all been getting deferred because nobody's been reviewing them so far.
We could create a separate review queue for it.
-- Best regards, Max Semenik ([[User:MaxSem]])
On Wed, Jul 21, 2010 at 4:04 AM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
Is someone planning on doing this? If not, who can do it? The sooner it's there, the better.
Cheers
-- Jeroen De Dauw
Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
I fixed one issue in r69675. $wgCodeReviewDeferredPaths applied to all repositories. So if we had a separate queue for Semantic things, it would have applied the main repo's deferral regexes. This has been fixed.
I'm also not sure how Code Review will handle a repository handling a subset of another repository. I'm pretty sure things will be ok, I only imagine it would just duplicate data (revs for SMW stuff would be imported for both repos). Still should be tested first though. Then we would need someone with repoadmin rights to set this up, I believe Brion or Tim can.
-Chad
2010/7/21 Chad innocentkiller@gmail.com:
I'm also not sure how Code Review will handle a repository handling a subset of another repository. I'm pretty sure things will be ok, I only imagine it would just duplicate data (revs for SMW stuff would be imported for both repos). Still should be tested first though. Then we would need someone with repoadmin rights to set this up, I believe Brion or Tim can.
Why would you want to do this? With the path search feature, it's extremely easy to pull up a list of revs touching a certain extension. I really don't see why the SMW review queue has to be separate from the main MW review queue on a technical level; of course it would be on a personal level, in that different people review different things, but we have that already for e.g. UsabilityInitiative. In practical terms, people who are familiar with the SMW codebase would start reviewing SMW revisions through our existing CodeReview setup, and the only thing we would have to do on a technical level is make sure those paths don't get auto-deferred.
Roan Kattouw (Catrope)
On Wed, Jul 21, 2010 at 8:38 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
2010/7/21 Chad innocentkiller@gmail.com:
I'm also not sure how Code Review will handle a repository handling a subset of another repository. I'm pretty sure things will be ok, I only imagine it would just duplicate data (revs for SMW stuff would be imported for both repos). Still should be tested first though. Then we would need someone with repoadmin rights to set this up, I believe Brion or Tim can.
Why would you want to do this? With the path search feature, it's extremely easy to pull up a list of revs touching a certain extension. I really don't see why the SMW review queue has to be separate from the main MW review queue on a technical level; of course it would be on a personal level, in that different people review different things, but we have that already for e.g. UsabilityInitiative. In practical terms, people who are familiar with the SMW codebase would start reviewing SMW revisions through our existing CodeReview setup, and the only thing we would have to do on a technical level is make sure those paths don't get auto-deferred.
Roan Kattouw (Catrope)
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
I agree with you here. They were just suggesting another route. Honestly, I don't really care either way :) The fix in r69675 is generally useful though, if repositories were segemented in that manner.
-Chad
2010/7/21 Chad innocentkiller@gmail.com:
I agree with you here. They were just suggesting another route. Honestly, I don't really care either way :) The fix in r69675 is generally useful though, if repositories were segemented in that manner.
...or if you happen to have repositories containing identical or similar paths.
Roan Kattouw (Catrope)
Hey,
I'm also fine either way. So if no separate queue is set up, I'd appropriate it if the semantic* commits where not marked as deferred from now on.
Cheers
-- Jeroen De Dauw * http://blog.bn2vs.com * http://wiki.bn2vs.com Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
On 21 July 2010 14:47, Chad innocentkiller@gmail.com wrote:
On Wed, Jul 21, 2010 at 8:38 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
2010/7/21 Chad innocentkiller@gmail.com:
I'm also not sure how Code Review will handle a repository handling a subset of another repository. I'm pretty sure things will be ok, I only imagine it would just duplicate data (revs for SMW stuff would be imported for both repos). Still should be tested first though. Then we would need someone with repoadmin rights to set this up, I believe Brion or Tim can.
Why would you want to do this? With the path search feature, it's extremely easy to pull up a list of revs touching a certain extension. I really don't see why the SMW review queue has to be separate from the main MW review queue on a technical level; of course it would be on a personal level, in that different people review different things, but we have that already for e.g. UsabilityInitiative. In practical terms, people who are familiar with the SMW codebase would start reviewing SMW revisions through our existing CodeReview setup, and the only thing we would have to do on a technical level is make sure those paths don't get auto-deferred.
Roan Kattouw (Catrope)
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
I agree with you here. They were just suggesting another route. Honestly, I don't really care either way :) The fix in r69675 is generally useful though, if repositories were segemented in that manner.
-Chad
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hey,
The 'semantic extensions' include Validator and Maps, as they are the base for Semantic Maps, so these should also not get deferred.
Cheers
-- Jeroen De Dauw * http://blog.bn2vs.com * http://wiki.bn2vs.com Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
On 21 July 2010 14:54, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
I'm also fine either way. So if no separate queue is set up, I'd appropriate it if the semantic* commits where not marked as deferred from now on.
Cheers
-- Jeroen De Dauw
Don't panic. Don't be evil. 50 72 6F 67 72 61 6D 6D 69 6E 67 20 34 20 6C 69 66 65! --
Roan Kattouw wrote:
Auto-deferral regexes for CodeReview were implemented in r63277 [1], and I deployed this feature three weeks ago. It allows us to set an array of regexes that will be matched against the path of each new commit; if one of them matches, the commit is automatically marked as 'deferred' instead of 'new'.
It should probably allow different status.
There are a few limitations to this implementation that are important to understand:
- it only matches paths, not commit summaries. This means
auto-deferring e.g. TranslateWiki exports is harder
- it only matches the root path of the commit, which is very often an
uninformative one like /trunk/phase3 , /trunk/extensions , /trunk or even / . This means you can't e.g. auto-defer all commits touching a certain file or path
Why not base it also in $rev->mPaths ? We don't touch so many files for it to be problematic on server resources, and an auto-defer for commits with all files matching "(^/trunk/phase3/languages/messages/|.i18n.php$)" would nicely skip all translatewiki updates.
Although a hook entry may be a more appropiate way.
It strikes me that a better solution is to fix whatever tools we're using to determine what still needs to be reviewed. If someone is checking all revisions marked as "new" and needs to mark things they won't review as "deferred" to get them off the list, maybe they should instead be checking all revisions marked as "new" from particular paths. Then explicit deferral will not be necessary, and projects like SMW can go ahead and use Code Review at their own pace without annoying anyone else.
2010/7/21 Aryeh Gregor Simetrical+wikilist@gmail.com:
It strikes me that a better solution is to fix whatever tools we're using to determine what still needs to be reviewed. If someone is checking all revisions marked as "new" and needs to mark things they won't review as "deferred" to get them off the list, maybe they should instead be checking all revisions marked as "new" from particular paths. Then explicit deferral will not be necessary, and projects like SMW can go ahead and use Code Review at their own pace without annoying anyone else.
As far as I know, this is exactly what happens in reality.
As I discussed with a few others at Wikimania, it'd be nice to take this one step further and allow multiple people to sign off on a revision, possibly with various types of sign-off, like: * I read the diff and it looks good * I tested this and seems to work * I reviewed the niche part of this rev that I'm an expert on * I am Tim Starling and I approve this message^Hrevision * ...
Roan Kattouw (Catrope)
On Wed, Jul 21, 2010 at 12:05 PM, Roan Kattouw roan.kattouw@gmail.com wrote:
As far as I know, this is exactly what happens in reality.
Then why do we need auto-deferral? Just let the things we don't care about stay new forever.
As I discussed with a few others at Wikimania, it'd be nice to take this one step further and allow multiple people to sign off on a revision, possibly with various types of sign-off, like:
- I read the diff and it looks good
- I tested this and seems to work
- I reviewed the niche part of this rev that I'm an expert on
- I am Tim Starling and I approve this message^Hrevision
- ...
I think this is a good idea. For simplicity, I'd keep it to one level, at least at first. The understanding should be that you should mark it reviewed if you're confident it's correct, and if obvious errors crop up later, it means people will informally give less weight to your review. Whether you tested it or just reviewed the diff should be up to you -- whatever you think it needs.
Aryeh Gregor wrote:
As I discussed with a few others at Wikimania, it'd be nice to take this one step further and allow multiple people to sign off on a revision, possibly with various types of sign-off, like:
- I read the diff and it looks good
- I tested this and seems to work
- I reviewed the niche part of this rev that I'm an expert on
- I am Tim Starling and I approve this message^Hrevision
- ...
I think this is a good idea. For simplicity, I'd keep it to one level, at least at first. The understanding should be that you should mark it reviewed if you're confident it's correct, and if obvious errors crop up later, it means people will informally give less weight to your review. Whether you tested it or just reviewed the diff should be up to you -- whatever you think it needs.
It's not the same. You can have different standards. I see revisions that are "apparently good", but marking as ok is "This revision is right" in my book, which in many cases would need actually testing it, checking spec, and so that I (lazily) don't do. So it keeps as new instead of as "lightly reviewed".
On Wed, Jul 21, 2010 at 5:12 PM, Platonides Platonides@gmail.com wrote:
It's not the same. You can have different standards. I see revisions that are "apparently good", but marking as ok is "This revision is right" in my book, which in many cases would need actually testing it, checking spec, and so that I (lazily) don't do. So it keeps as new instead of as "lightly reviewed".
Is it useful to know that something is lightly reviewed?
2010/7/21 Aryeh Gregor Simetrical+wikilist@gmail.com:
On Wed, Jul 21, 2010 at 5:12 PM, Platonides Platonides@gmail.com wrote:
It's not the same. You can have different standards. I see revisions that are "apparently good", but marking as ok is "This revision is right" in my book, which in many cases would need actually testing it, checking spec, and so that I (lazily) don't do. So it keeps as new instead of as "lightly reviewed".
Is it useful to know that something is lightly reviewed?
I would say so, yes. The more people 'lightly review' something, the less likely it is to have obvious-ish flaws. However, most of the time the name of the person that reviewed it is the most significant piece of information, assuming you know the reviewers well.
Roan Kattouw (Catrope)
On Wed, Jul 21, 2010 at 6:51 PM, Roan Kattouw roan.kattouw@gmail.com wrote:
I would say so, yes. The more people 'lightly review' something, the less likely it is to have obvious-ish flaws. However, most of the time the name of the person that reviewed it is the most significant piece of information, assuming you know the reviewers well.
Maybe, but I don't think we need actual UI for indicating this. What we'd really want to know is whether at least one trusted person has carefully looked at the code and really thinks it's correct. IMO, we should have a system that encourages people to commit to a change's correctness, in a way that they can be held accountable (via people paying less attention to their review) if they miss things too often. Having a secondary review level that doesn't really mean much because you only superficially reviewed the code will encourage people to use that level of review so that they can't really be blamed if the commit is actually bad, rather than taking responsibility for it.
wikitech-l@lists.wikimedia.org