jenkins-bot has submitted this change and it was merged.
Change subject: Move duplicate code to UsageAccumulator classes
......................................................................
Move duplicate code to UsageAccumulator classes
This patch does not finally solve the problem of duplicate code. It
moves the code to a place where it fits better, in my opinion. There
will be an othe rpatchj on top of this.
Change-Id: I9107b2701a958c89b46005f63caf19e0bd7e4a24
---
M client/includes/DataAccess/StatementTransclusionInteractor.php
M client/includes/Usage/HashUsageAccumulator.php
M client/includes/Usage/ParserOutputUsageAccumulator.php
M client/includes/Usage/UsageAccumulator.php
M client/includes/scribunto/SnakSerializationRenderer.php
M
client/tests/phpunit/includes/DataAccess/PropertyParserFunction/LanguageAwareRendererTest.php
M
client/tests/phpunit/includes/DataAccess/StatementTransclusionInteractorTest.php
M client/tests/phpunit/includes/Usage/HashUsageAccumulatorTest.php
M client/tests/phpunit/includes/Usage/UsageAccumulatorContractTester.php
9 files changed, 121 insertions(+), 134 deletions(-)
Approvals:
Hoo man: Looks good to me, approved
jenkins-bot: Verified
Objections:
Daniel Kinzler: There's a problem with this change, please improve
diff --git a/client/includes/DataAccess/StatementTransclusionInteractor.php
b/client/includes/DataAccess/StatementTransclusionInteractor.php
index ab134f4..0008c07 100644
--- a/client/includes/DataAccess/StatementTransclusionInteractor.php
+++ b/client/includes/DataAccess/StatementTransclusionInteractor.php
@@ -4,11 +4,7 @@
use Language;
use Wikibase\Client\Usage\UsageAccumulator;
-use Wikibase\DataAccess\PropertyIdResolver;
-use Wikibase\DataAccess\SnaksFinder;
use Wikibase\DataModel\Entity\EntityId;
-use Wikibase\DataModel\Entity\EntityIdValue;
-use Wikibase\DataModel\Snak\PropertyValueSnak;
use Wikibase\DataModel\Snak\Snak;
use Wikibase\DataModel\StatementListProvider;
use Wikibase\Lib\PropertyLabelNotResolvedException;
@@ -81,7 +77,12 @@
* @throws PropertyLabelNotResolvedException
* @return string
*/
- public function render( EntityId $entityId, UsageAccumulator
$usageAccumulator, $propertyLabelOrId, $acceptableRanks = null ) {
+ public function render(
+ EntityId $entityId,
+ UsageAccumulator $usageAccumulator,
+ $propertyLabelOrId,
+ $acceptableRanks = null
+ ) {
$entity = $this->entityLookup->getEntity( $entityId );
if ( !$entity instanceof StatementListProvider ) {
@@ -98,7 +99,8 @@
$propertyId,
$acceptableRanks
);
- $this->trackUsage( $snaks, $usageAccumulator );
+
+ $usageAccumulator->addLabelUsageForSnaks( $snaks );
return $this->formatSnaks( $snaks );
}
@@ -116,28 +118,6 @@
}
return $this->language->commaList( $formattedValues );
- }
-
- /**
- * @param Snak[] $snaks
- * @param UsageAccumulator $usageAccumulator
- */
- private function trackUsage( array $snaks, UsageAccumulator
$usageAccumulator ) {
- // Note: we track any EntityIdValue as a label usage.
- // This is making assumptions about what the respective
formatter actually does.
- // Ideally, the formatter itself would perform the tracking,
but that seems nasty to model.
-
- foreach ( $snaks as $snak ) {
- if ( !( $snak instanceof PropertyValueSnak) ) {
- continue;
- }
-
- $value = $snak->getDataValue();
-
- if ( $value instanceof EntityIdValue ) {
- $usageAccumulator->addLabelUsage(
$value->getEntityId() );
- }
- }
}
}
diff --git a/client/includes/Usage/HashUsageAccumulator.php
b/client/includes/Usage/HashUsageAccumulator.php
index 7bc0637..e819b92 100644
--- a/client/includes/Usage/HashUsageAccumulator.php
+++ b/client/includes/Usage/HashUsageAccumulator.php
@@ -3,6 +3,9 @@
namespace Wikibase\Client\Usage;
use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\DataModel\Entity\EntityIdValue;
+use Wikibase\DataModel\Snak\PropertyValueSnak;
+use Wikibase\DataModel\Snak\Snak;
/**
* This implementation of the UsageAccumulator interface simply wraps
@@ -41,6 +44,23 @@
}
/**
+ * @see UsageAccumulator::addLabelUsageForSnaks
+ *
+ * @param Snak[] $snaks
+ */
+ public function addLabelUsageForSnaks( array $snaks ) {
+ foreach ( $snaks as $snak ) {
+ if ( $snak instanceof PropertyValueSnak ) {
+ $value = $snak->getDataValue();
+
+ if ( $value instanceof EntityIdValue ) {
+ $this->addLabelUsage(
$value->getEntityId() );
+ }
+ }
+ }
+ }
+
+ /**
* @see UsageAccumulator::addLabelUsage
*
* @param EntityId $id
diff --git a/client/includes/Usage/ParserOutputUsageAccumulator.php
b/client/includes/Usage/ParserOutputUsageAccumulator.php
index 59b1528..d89a57f 100644
--- a/client/includes/Usage/ParserOutputUsageAccumulator.php
+++ b/client/includes/Usage/ParserOutputUsageAccumulator.php
@@ -4,6 +4,9 @@
use ParserOutput;
use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\DataModel\Entity\EntityIdValue;
+use Wikibase\DataModel\Snak\PropertyValueSnak;
+use Wikibase\DataModel\Snak\Snak;
/**
* This implementation of the UsageAccumulator interface acts as a wrapper
around
@@ -51,6 +54,23 @@
}
/**
+ * @see UsageAccumulator::addLabelUsageForSnaks
+ *
+ * @param Snak[] $snaks
+ */
+ public function addLabelUsageForSnaks( array $snaks ) {
+ foreach ( $snaks as $snak ) {
+ if ( $snak instanceof PropertyValueSnak ) {
+ $value = $snak->getDataValue();
+
+ if ( $value instanceof EntityIdValue ) {
+ $this->addLabelUsage(
$value->getEntityId() );
+ }
+ }
+ }
+ }
+
+ /**
* @see UsageAccumulator::addLabelUsage
*
* @param EntityId $id
diff --git a/client/includes/Usage/UsageAccumulator.php
b/client/includes/Usage/UsageAccumulator.php
index d6ff542..bffcf5c 100644
--- a/client/includes/Usage/UsageAccumulator.php
+++ b/client/includes/Usage/UsageAccumulator.php
@@ -3,6 +3,7 @@
namespace Wikibase\Client\Usage;
use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\DataModel\Snak\Snak;
/**
* Interface for objects accumulating usage tracking information for a given
page.
@@ -13,7 +14,19 @@
interface UsageAccumulator {
/**
- * Registers the usage an entity's label (in the local content
language).
+ * Registers the usage of entity's labels (in the local content
language), if the provided
+ * snaks are PropertyValueSnaks that contain EntityIdValues.
+ *
+ * @note We track any EntityIdValue as a label usage. This is making
assumptions about what the
+ * respective formatter actually does. Ideally, the formatter itself
would perform the tracking,
+ * but that seems nasty to model.
+ *
+ * @param Snak[] $snaks
+ */
+ public function addLabelUsageForSnaks( array $snaks );
+
+ /**
+ * Registers the usage of an entity's label (in the local content
language).
*
* @param EntityId $id
*/
diff --git a/client/includes/scribunto/SnakSerializationRenderer.php
b/client/includes/scribunto/SnakSerializationRenderer.php
index 9e30dac..8432f4f 100644
--- a/client/includes/scribunto/SnakSerializationRenderer.php
+++ b/client/includes/scribunto/SnakSerializationRenderer.php
@@ -4,10 +4,9 @@
use Language;
use Wikibase\Client\Usage\UsageAccumulator;
-use Wikibase\DataModel\Deserializers\SnakListDeserializer;
use Wikibase\DataModel\Deserializers\SnakDeserializer;
-use Wikibase\DataModel\Snak\PropertyValueSnak;
-use Wikibase\DataModel\Entity\EntityIdValue;
+use Wikibase\DataModel\Deserializers\SnakListDeserializer;
+use Wikibase\DataModel\Snak\Snak;
use Wikibase\Lib\SnakFormatter;
/**
@@ -78,7 +77,7 @@
public function renderSnak( array $snakSerialization ) {
$snak = $this->snakDeserializer->deserialize(
$snakSerialization );
- $this->trackUsage( array( $snak ) );
+ $this->usageAccumulator->addLabelUsageForSnaks( array( $snak )
);
return $this->snakFormatter->formatSnak( $snak );
}
@@ -100,7 +99,9 @@
}
$snaks = iterator_to_array( $snaks );
- $this->trackUsage( $snaks );
+
+ $this->usageAccumulator->addLabelUsageForSnaks( $snaks );
+
return $this->formatSnakList( $snaks );
}
@@ -118,25 +119,4 @@
return $this->language->commaList( $formattedValues );
}
- /**
- * @todo Share code with LanguageAwareRenderer::trackUsage
- * @param Snak[] $snaks
- */
- private function trackUsage( array $snaks ) {
- // Note: we track any EntityIdValue as a label usage.
- // This is making assumptions about what the respective
formatter actually does.
- // Ideally, the formatter itself would perform the tracking,
but that seems nasty to model.
-
- foreach ( $snaks as $snak ) {
- if ( !( $snak instanceof PropertyValueSnak ) ) {
- continue;
- }
-
- $value = $snak->getDataValue();
-
- if ( $value instanceof EntityIdValue ) {
- $this->usageAccumulator->addLabelUsage(
$value->getEntityId() );
- }
- }
- }
-}
\ No newline at end of file
+}
diff --git
a/client/tests/phpunit/includes/DataAccess/PropertyParserFunction/LanguageAwareRendererTest.php
b/client/tests/phpunit/includes/DataAccess/PropertyParserFunction/LanguageAwareRendererTest.php
index af76e2c..b1d0594 100644
---
a/client/tests/phpunit/includes/DataAccess/PropertyParserFunction/LanguageAwareRendererTest.php
+++
b/client/tests/phpunit/includes/DataAccess/PropertyParserFunction/LanguageAwareRendererTest.php
@@ -5,12 +5,12 @@
use DataValues\StringValue;
use Language;
use Wikibase\Client\Usage\EntityUsage;
+use Wikibase\Client\Usage\HashUsageAccumulator;
use Wikibase\Client\Usage\UsageAccumulator;
-use Wikibase\DataAccess\StatementTransclusionInteractor;
use Wikibase\DataAccess\PropertyIdResolver;
use Wikibase\DataAccess\PropertyParserFunction\LanguageAwareRenderer;
use Wikibase\DataAccess\SnaksFinder;
-use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\DataAccess\StatementTransclusionInteractor;
use Wikibase\DataModel\Entity\EntityIdValue;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\Entity\PropertyId;
@@ -34,41 +34,19 @@
class LanguageAwareRendererTest extends \PHPUnit_Framework_TestCase {
/**
- * @param array $usages
- *
- * @return UsageAccumulator
- */
- private function getUsageAccumulator( array &$usages ) {
- $mock = $this->getMockBuilder(
'Wikibase\Client\Usage\UsageAccumulator' )
- ->disableOriginalConstructor()
- ->getMock();
-
- $mock->expects( $this->any() )
- ->method( 'addLabelUsage' )
- ->will( $this->returnCallback(
- function ( EntityId $id ) use ( &$usages ) {
- $usages[] = new EntityUsage( $id,
EntityUsage::LABEL_USAGE );
- }
- ) );
-
- $mock->expects( $this->never() )
- ->method( 'addAllUsage' );
-
- $mock->expects( $this->never() )
- ->method( 'addSiteLinksUsage' );
-
- return $mock;
- }
-
- /**
* @param PropertyIdResolver $propertyIdResolver
* @param SnaksFinder $snaksFinder
* @param string $languageCode
- * @param array &$usages
+ * @param UsageAccumulator|null $usageAccumulator
*
* @return LanguageAwareRenderer
*/
- private function getRenderer( PropertyIdResolver $propertyIdResolver,
SnaksFinder $snaksFinder, $languageCode, array &$usages = array() ) {
+ private function getRenderer(
+ PropertyIdResolver $propertyIdResolver,
+ SnaksFinder $snaksFinder,
+ $languageCode,
+ UsageAccumulator $usageAccumulator = null
+ ) {
$targetLanguage = Language::factory( $languageCode );
$entityStatementsRenderer = new StatementTransclusionInteractor(
@@ -82,7 +60,7 @@
return new LanguageAwareRenderer(
$targetLanguage,
$entityStatementsRenderer,
- $this->getUsageAccumulator( $usages )
+ $usageAccumulator ?: new HashUsageAccumulator()
);
}
@@ -93,8 +71,11 @@
'Q42$2' => new PropertyValueSnak( $propertyId, new
StringValue( 'two kittens!!' ) )
);
- $usages = array();
- $renderer = $this->getRenderer( $this->getPropertyIdResolver(),
$this->getSnaksFinder( $snaks ), 'en', $usages );
+ $renderer = $this->getRenderer(
+ $this->getPropertyIdResolver(),
+ $this->getSnaksFinder( $snaks ),
+ 'en'
+ );
$q42 = new ItemId( 'Q42' );
$result = $renderer->render( $q42, 'p1337' );
@@ -112,8 +93,13 @@
'Q42$23' => new PropertyValueSnak( $propertyId, new
EntityIdValue( $q23 ) )
);
- $usages = array();
- $renderer = $this->getRenderer( $this->getPropertyIdResolver(),
$this->getSnaksFinder( $snaks ), 'en', $usages );
+ $accumulator = new HashUsageAccumulator();
+ $renderer = $this->getRenderer(
+ $this->getPropertyIdResolver(),
+ $this->getSnaksFinder( $snaks ),
+ 'en',
+ $accumulator
+ );
$q42 = new ItemId( 'Q42' );
$renderer->render( $q42, 'p1337' );
@@ -123,7 +109,7 @@
new EntityUsage( $q23, EntityUsage::LABEL_USAGE ),
);
- $this->assertSameUsages( $expectedUsage, $usages );
+ $this->assertSameUsages( $expectedUsage,
$accumulator->getUsages() );
}
/**
@@ -188,7 +174,8 @@
$renderer = $this->getRenderer(
$this->getPropertyIdResolverForPropertyNotFound(),
$this->getSnaksFinder( array() ),
- 'qqx' );
+ 'qqx'
+ );
$result = $renderer->render( new ItemId( 'Q4' ), 'invalidLabel'
);
$this->assertRegExp(
diff --git
a/client/tests/phpunit/includes/DataAccess/StatementTransclusionInteractorTest.php
b/client/tests/phpunit/includes/DataAccess/StatementTransclusionInteractorTest.php
index 84994c0..c076abf 100644
---
a/client/tests/phpunit/includes/DataAccess/StatementTransclusionInteractorTest.php
+++
b/client/tests/phpunit/includes/DataAccess/StatementTransclusionInteractorTest.php
@@ -2,15 +2,14 @@
namespace Wikibase\Client\Tests\DataAccess\PropertyParserFunction;
+use DataValues\StringValue;
use Language;
use PHPUnit_Framework_TestCase;
-use DataValues\StringValue;
use Wikibase\Client\Usage\EntityUsage;
-use Wikibase\Client\Usage\UsageAccumulator;
-use Wikibase\DataAccess\StatementTransclusionInteractor;
+use Wikibase\Client\Usage\HashUsageAccumulator;
use Wikibase\DataAccess\PropertyIdResolver;
use Wikibase\DataAccess\SnaksFinder;
-use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\DataAccess\StatementTransclusionInteractor;
use Wikibase\DataModel\Entity\EntityIdValue;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\Entity\PropertyId;
@@ -34,41 +33,17 @@
class StatementTransclusionInteractorTest extends PHPUnit_Framework_TestCase {
/**
- * @param array $usages
- *
- * @return UsageAccumulator
- */
- private function getUsageAccumulator( array &$usages ) {
- $mock = $this->getMockBuilder(
'Wikibase\Client\Usage\UsageAccumulator' )
- ->disableOriginalConstructor()
- ->getMock();
-
- $mock->expects( $this->any() )
- ->method( 'addLabelUsage' )
- ->will( $this->returnCallback(
- function ( EntityId $id ) use ( &$usages ) {
- $usages[] = new EntityUsage( $id,
EntityUsage::LABEL_USAGE );
- }
- ) );
-
- $mock->expects( $this->never() )
- ->method( 'addAllUsage' );
-
- $mock->expects( $this->never() )
- ->method( 'addSiteLinksUsage' );
-
- return $mock;
- }
-
- /**
* @param PropertyIdResolver $propertyIdResolver
* @param SnaksFinder $snaksFinder
* @param string $languageCode
- * @param array &$usages
*
* @return StatementTransclusionInteractor
*/
- private function getInteractor( PropertyIdResolver $propertyIdResolver,
SnaksFinder $snaksFinder, $languageCode ) {
+ private function getInteractor(
+ PropertyIdResolver $propertyIdResolver,
+ SnaksFinder $snaksFinder,
+ $languageCode
+ ) {
$targetLanguage = Language::factory( $languageCode );
return new StatementTransclusionInteractor(
@@ -87,7 +62,6 @@
'Q42$2' => new PropertyValueSnak( $propertyId, new
StringValue( 'two kittens!!' ) )
);
- $usages = array();
$renderer = $this->getInteractor(
$this->getPropertyIdResolver(),
$this->getSnaksFinder( $snaks ),
@@ -95,14 +69,13 @@
);
$q42 = new ItemId( 'Q42' );
- $result = $renderer->render( $q42, $this->getUsageAccumulator(
$usages ) , 'p1337' );
+ $result = $renderer->render( $q42, new HashUsageAccumulator(),
'p1337' );
$expected = 'a kitten!, two kittens!!';
$this->assertEquals( $expected, $result );
}
public function testRender_PropertyLabelNotResolvedException() {
- $usages = array();
$renderer = $this->getInteractor(
$this->getPropertyIdResolverForPropertyNotFound(),
$this->getSnaksFinder( array() ),
@@ -110,7 +83,7 @@
);
$this->setExpectedException(
'Wikibase\Lib\PropertyLabelNotResolvedException' );
- $renderer->render( new ItemId( 'Q42' ),
$this->getUsageAccumulator( $usages ), 'blah' );
+ $renderer->render( new ItemId( 'Q42' ), new
HashUsageAccumulator(), 'blah' );
}
public function testRender_trackUsage() {
@@ -122,18 +95,18 @@
'Q42$23' => new PropertyValueSnak( $propertyId, new
EntityIdValue( $q23 ) )
);
- $usages = array();
+ $accumulator = new HashUsageAccumulator();
$renderer = $this->getInteractor(
$this->getPropertyIdResolver(), $this->getSnaksFinder( $snaks ), 'en' );
$q42 = new ItemId( 'Q42' );
- $renderer->render( $q42, $this->getUsageAccumulator( $usages ),
'p1337' );
+ $renderer->render( $q42, $accumulator, 'p1337' );
$expectedUsage = array(
new EntityUsage( $q22, EntityUsage::LABEL_USAGE ),
new EntityUsage( $q23, EntityUsage::LABEL_USAGE ),
);
- $this->assertSameUsages( $expectedUsage, $usages );
+ $this->assertSameUsages( $expectedUsage,
$accumulator->getUsages() );
}
/**
diff --git a/client/tests/phpunit/includes/Usage/HashUsageAccumulatorTest.php
b/client/tests/phpunit/includes/Usage/HashUsageAccumulatorTest.php
index 7d6a9e6..32ccfd1 100644
--- a/client/tests/phpunit/includes/Usage/HashUsageAccumulatorTest.php
+++ b/client/tests/phpunit/includes/Usage/HashUsageAccumulatorTest.php
@@ -1,4 +1,5 @@
<?php
+
namespace Wikibase\Client\Usage\Tests;
use Wikibase\Client\Usage\HashUsageAccumulator;
diff --git
a/client/tests/phpunit/includes/Usage/UsageAccumulatorContractTester.php
b/client/tests/phpunit/includes/Usage/UsageAccumulatorContractTester.php
index ea033eb..e528d39 100644
--- a/client/tests/phpunit/includes/Usage/UsageAccumulatorContractTester.php
+++ b/client/tests/phpunit/includes/Usage/UsageAccumulatorContractTester.php
@@ -1,10 +1,14 @@
<?php
+
namespace Wikibase\Client\Usage\Tests;
use PHPUnit_Framework_Assert as Assert;
use Wikibase\Client\Usage\EntityUsage;
use Wikibase\Client\Usage\UsageAccumulator;
+use Wikibase\DataModel\Entity\EntityIdValue;
use Wikibase\DataModel\Entity\ItemId;
+use Wikibase\DataModel\Entity\PropertyId;
+use Wikibase\DataModel\Snak\PropertyValueSnak;
/**
* Contract tester for implementations of the UsageAccumulator interface
@@ -30,11 +34,19 @@
public function testAddGetUsage() {
$q2 = new ItemId( 'Q2' );
$q3 = new ItemId( 'Q3' );
+ $q4 = new ItemId( 'Q4' );
+ // FIXME: Split into separate tests.
+ // FIXME: Why is OTHER_USAGE missing?
$this->usageAccumulator->addSiteLinksUsage( $q2 );
$this->usageAccumulator->addLabelUsage( $q2 );
$this->usageAccumulator->addTitleUsage( $q2 );
$this->usageAccumulator->addAllUsage( $q3 );
+
+ // Testing convenience methods
+ $this->usageAccumulator->addLabelUsageForSnaks( array(
+ new PropertyValueSnak( new PropertyId( 'P4' ), new
EntityIdValue( $q4 ) ),
+ ) );
$usage = $this->usageAccumulator->getUsages();
@@ -43,6 +55,7 @@
new EntityUsage( $q2, EntityUsage::LABEL_USAGE ),
new EntityUsage( $q2, EntityUsage::TITLE_USAGE ),
new EntityUsage( $q3, EntityUsage::ALL_USAGE ),
+ new EntityUsage( $q4, EntityUsage::LABEL_USAGE ),
);
$this->assertSameUsages( $expected, $usage );
--
To view, visit https://gerrit.wikimedia.org/r/191335
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9107b2701a958c89b46005f63caf19e0bd7e4a24
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Hoo man <[email protected]>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
