Re: [Wikitech-l] [MediaWiki-CVS] SVN: [41458] trunk/extensions/CodeReview - Wikitech-l

1 Oct 2008

brion(a)svn.wikimedia.org schreef:
...
  Revision: 41458
 Author:   brion
 Date:     2008-09-30 23:04:24 +0000 (Tue, 30 Sep 2008)

 Log Message:
 -----------
 Add an API module that can be POSTed to to trigger a codereview repo update

 +		$repo = CodeRepository::newFromName( $params['repo'] );
 +		if( !$repo ){
 +			throw new MWException( "Invalid repo {$args[0]}" );
 +		} Throwing an MWException on invalid input is not very nice. You should 
really use something like:

$this->dieUsage("Invalid repo ``{$args[0]}''",
'invalidrepo');

If you have some kind of array containing all valid repos lying around, 
you could use that in getAllowedParams() so the repo parameter will 
automatically be validated by extractRequestParams().
...
  +		$endRev = intval( $params['rev'] );
You can also set the rev parameter to be an integer in 
getAllowedParams(), so you won't have to use intval() (and so 
non-integers will be rejected instead of being silently converted to 0).
...
  +		if( $lastStoredRev >= $endRev ) {
 +			// Nothing to do, we're up to date.
 +			return;
 +		} You should still output *something* here (see also below).
...
  +		// fixme: this could be a lot? Yeah, you
gotta thing about max exec time here. What you could do is 
just process the first 50 or so and tell the client where you stopped, 
so they can issue another request for the next 50.
...
  +		$log = $svn->getLog( '', $lastStoredRev
+ 1, $endRev );
 +		if( !$log ) {
 +			throw new MWException( "Something awry..." );
 +		} I don't know in which circumstances $log would be empty or false, but 
unless it only happens in really crazy cases you should use dieUsage() 
here as well.
...
  +		foreach( $log as $data ) {
 +			$codeRev = CodeRevision::newFromSvn( $repo, $data );
 +			$codeRev->save();
 +			// would be nice to output something but the api code is weird
 +			// and i don't feel like figuring it out right now :)
 +		} You probably wanna do something like this:

$data = array();
foreach($log as $entry)
{
        // Do whatever you need to do
        $data['saved'][] = array('revid' => $revid, /* More properties

here */);
}
$data['continue'] = $revidToContinueFrom;
// Tell the XML formatter to use <rev> tags for the $data['saved'] array
// You need to call this function on every array with integer indices,
// or ApiFormatXml will scream and die
$this->getResult()->setIndexedTagName($data['saved'], 'rev');
// Add $data on the top level (that's what the null does) in a
// <codeupdate> tag (which is what getModuleName() returns)
$this->getResult()->addValue(null, $this->getModuleName(), $data);

The XML this will return will look like:
<api>
  <codeupdate continue="56">
    <saved>
      <rev revid="54" />
      <rev revid="55" />
      <rev revid="56" />
    </saved>
  </codeupdate>
</api>
 
...
  +	public function getAllowedParams() {
 +		return array(
 +			'repo' => null,
 +			'rev' => null );
 +	} A better getAllowedParams() array would probably be:

// This assumes $wgAvailableRepos is an array of
// (surprise) available repos. If there is no such
// array, you should still use 'repo' => null like
// you did initially.
array(
        'repo' => array(
                ApiBase::PARAM_TYPE => $wgAvailableRepos
        ),
        'rev' => array(
                ApiBase::PARAM_TYPE => 'integer'
        )
)

Roan Kattouw (Catrope)