CodeReview auto-deferral regexes

List overview All Threads
Download

newer

older

Salutations From a Computer...

Re: [Wikitech-l] "Take me back"...

Roan Kattouw

20 Jul 2010 20 Jul '10

1:38 p.m.

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

Show replies by date

Daniel Kinzler

20 Jul 20 Jul

1:46 p.m.

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

Chad

3:10 p.m.

On Tue, Jul 20, 2010 at 7:46 AM, Daniel Kinzler <daniel(a)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?

Jeroen De Dauw

3:16 p.m.

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(a)gmail.com> wrote:

...

On Tue, Jul 20, 2010 at 7:46 AM, Daniel Kinzler <daniel(a)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

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(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Chad

3:20 p.m.

On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw <jeroendedauw(a)gmail.com> wrote:

...

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

Max Semenik

5:20 p.m.

On 20.07.2010, 17:20 Chad wrote:

...

On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw <jeroendedauw(a)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]])

Jeroen De Dauw

5:25 p.m.

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(a)gmail.com> wrote:

...

On 20.07.2010, 17:20 Chad wrote:

On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw <jeroendedauw(a)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(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Markus Krötzsch

7:33 p.m.

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 * 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(a)gmail.com> wrote:

On 20.07.2010, 17:20 Chad wrote:

On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw<jeroendedauw(a)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.

_______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Jeroen De Dauw

21 Jul 21 Jul

10:04 a.m.

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(a)gmail.com> wrote:

...

On 20.07.2010, 17:20 Chad wrote:

On Tue, Jul 20, 2010 at 9:16 AM, Jeroen De Dauw <jeroendedauw(a)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]])

Chad

1:58 p.m.

On Wed, Jul 21, 2010 at 4:04 AM, Jeroen De Dauw <jeroendedauw(a)gmail.com> wrote:

...

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

Roan Kattouw

2:38 p.m.

2010/7/21 Chad <innocentkiller(a)gmail.com>om>:

...

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)

Chad

2:47 p.m.

On Wed, Jul 21, 2010 at 8:38 AM, Roan Kattouw <roan.kattouw(a)gmail.com> wrote:

...

2010/7/21 Chad <innocentkiller(a)gmail.com>om>:

Roan Kattouw

2:50 p.m.

2010/7/21 Chad <innocentkiller(a)gmail.com>om>:

...

...or if you happen to have repositories containing identical or similar paths. Roan Kattouw (Catrope)

Jeroen De Dauw

2:54 p.m.

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(a)gmail.com> wrote:

...

On Wed, Jul 21, 2010 at 8:38 AM, Roan Kattouw <roan.kattouw(a)gmail.com> wrote:

2010/7/21 Chad <innocentkiller(a)gmail.com>om>:

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(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Jeroen De Dauw

3:10 p.m.

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(a)gmail.com> wrote:

...

Platonides

20 Jul 20 Jul

8:01 p.m.

Roan Kattouw wrote:

...

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.

Aryeh Gregor

21 Jul 21 Jul

5:13 p.m.

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.

Roan Kattouw

6:05 p.m.

2010/7/21 Aryeh Gregor <Simetrical+wikilist(a)gmail.com>om>:

...

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)

Aryeh Gregor

6:48 p.m.

On Wed, Jul 21, 2010 at 12:05 PM, Roan Kattouw <roan.kattouw(a)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.

Platonides

11:12 p.m.

Aryeh Gregor 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".

Aryeh Gregor

11:58 p.m.

On Wed, Jul 21, 2010 at 5:12 PM, Platonides <Platonides(a)gmail.com> wrote:

...

Is it useful to know that something is lightly reviewed?

Roan Kattouw

22 Jul 22 Jul

12:51 a.m.

2010/7/21 Aryeh Gregor <Simetrical+wikilist(a)gmail.com>om>:

...

On Wed, Jul 21, 2010 at 5:12 PM, Platonides <Platonides(a)gmail.com> wrote:

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)

Aryeh Gregor

5:18 p.m.

On Wed, Jul 21, 2010 at 6:51 PM, Roan Kattouw <roan.kattouw(a)gmail.com> wrote:

...

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.

5061

days inactive

5063

days old

wikitech-l@lists.wikimedia.org

Manage subscription

22 comments

8 participants

tags (0)

participants (8)

Aryeh Gregor
Chad
Daniel Kinzler
Jeroen De Dauw
Markus Krötzsch
Max Semenik
Platonides
Roan Kattouw