Daniel Kinzler has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/347621 )
Change subject: Simplify LexemeDiffs ...................................................................... Simplify LexemeDiffs Language and lexicalCategory diffs should be atomic. This also adds the previously missing LexemeDiffTest. Change-Id: If46efb61a0abacc7632b125643eb749946143785 --- M src/DataModel/Services/Diff/LexemeDiff.php M src/DataModel/Services/Diff/LexemeDiffer.php M src/DataModel/Services/Diff/LexemePatcher.php A tests/phpunit/composer/DataModel/Services/Diff/LexemeDiffTest.php M tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferTest.php M tests/phpunit/composer/DataModel/Services/Diff/LexemePatcherTest.php 6 files changed, 123 insertions(+), 54 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikibaseLexeme refs/changes/21/347621/1 diff --git a/src/DataModel/Services/Diff/LexemeDiff.php b/src/DataModel/Services/Diff/LexemeDiff.php index ee9f4e3..d287721 100644 --- a/src/DataModel/Services/Diff/LexemeDiff.php +++ b/src/DataModel/Services/Diff/LexemeDiff.php @@ -13,6 +13,7 @@ * * @license GPL-2.0+ * @author Amir Sarabadani <[email protected]> + * @author Daniel Kinzler */ class LexemeDiff extends EntityDiff { @@ -21,9 +22,6 @@ */ public function __construct( array $operations = [] ) { $this->fixSubstructureDiff( $operations, 'lemmas' ); - $this->fixSubstructureDiff( $operations, 'lexicalCategory' ); - $this->fixSubstructureDiff( $operations, 'language' ); - $this->fixSubstructureDiff( $operations, 'claim' ); parent::__construct( $operations ); } @@ -38,31 +36,24 @@ } /** - * Returns a Diff object with the lexical category differences. - * - * @return Diff - */ - public function getLexicalCategoryDiff() { - return isset( $this['lexicalCategory'] ) ? $this['lexicalCategory'] : new Diff( [], true ); - } - - /** - * Returns a Diff object with the language differences. - * - * @return Diff - */ - public function getLanguageDiff() { - return isset( $this['language'] ) ? $this['language'] : new Diff( [], true ); - } - - /** * Returns if there are any changes (equivalent to: any differences between the entities). * * @return bool */ public function isEmpty() { - return $this->getLemmasDiff()->isEmpty() - && $this->getClaimsDiff()->isEmpty(); + return parent::isEmpty() + && !isset( $this['language'] ) + && !isset( $this['lexicalCategory'] ) + && $this->getLemmasDiff()->isEmpty(); + } + + /** + * @see DiffOp::getType + * + * @return string + */ + public function getType() { + return 'diff/lexeme'; } } diff --git a/src/DataModel/Services/Diff/LexemeDiffer.php b/src/DataModel/Services/Diff/LexemeDiffer.php index b174a5b..dd57fe3 100644 --- a/src/DataModel/Services/Diff/LexemeDiffer.php +++ b/src/DataModel/Services/Diff/LexemeDiffer.php @@ -95,13 +95,13 @@ } try { - $array['lexicalCategory'] = [ 'id' => $lexeme->getLexicalCategory()->getSerialization() ]; + $array['lexicalCategory'] = $lexeme->getLexicalCategory(); } catch ( UnexpectedValueException $ex ) { // It's fine to skip uninitialized properties in a diff } try { - $array['language'] = [ 'id' => $lexeme->getLanguage()->getSerialization() ]; + $array['language'] = $lexeme->getLanguage(); } catch ( UnexpectedValueException $ex ) { // It's fine to skip uninitialized properties in a diff } diff --git a/src/DataModel/Services/Diff/LexemePatcher.php b/src/DataModel/Services/Diff/LexemePatcher.php index 0ec031c..3475912 100644 --- a/src/DataModel/Services/Diff/LexemePatcher.php +++ b/src/DataModel/Services/Diff/LexemePatcher.php @@ -3,6 +3,7 @@ namespace Wikibase\Lexeme\DataModel\Services\Diff; use Diff\DiffOp\Diff\Diff; +use Diff\DiffOp\DiffOp; use Diff\DiffOp\DiffOpAdd; use Diff\DiffOp\DiffOpChange; use Diff\DiffOp\DiffOpRemove; @@ -72,44 +73,42 @@ $patch->getClaimsDiff() ); - $itemId = $this->getPatchedItemId( $patch->getLexicalCategoryDiff() ); - if ( $itemId !== false ) { - $entity->setLexicalCategory( $itemId ); + if ( isset( $patch['lexicalCategory'] ) ) { + $lexicalCategory = $this->getPatchedItemId( $patch['lexicalCategory'] ); + $entity->setLexicalCategory( $lexicalCategory ); } - $itemId = $this->getPatchedItemId( $patch->getLanguageDiff() ); - if ( $itemId !== false ) { - $entity->setLanguage( $itemId ); + if ( isset( $patch['language'] ) ) { + $language = $this->getPatchedItemId( $patch['language'] ); + $entity->setLanguage( $language ); } } /** - * @param Diff $patch + * @param DiffOp $diffOp * * @throws PatcherException - * @return ItemId|null|false False in case the diff is valid, but does not contain a change. + * @return ItemId */ - private function getPatchedItemId( Diff $patch ) { - if ( $patch->isEmpty() ) { - return false; - } - - $diffOp = $patch['id']; + private function getPatchedItemId( DiffOp $diffOp ) { switch ( true ) { case $diffOp instanceof DiffOpAdd: /** @var DiffOpAdd $diffOp */ - return new ItemId( $diffOp->getNewValue() ); + $id = $diffOp->getNewValue(); + break; case $diffOp instanceof DiffOpChange: /** @var DiffOpChange $diffOp */ - return new ItemId( $diffOp->getNewValue() ); + $id = $diffOp->getNewValue(); + break; - case $diffOp instanceof DiffOpRemove: - return null; + default: + throw new PatcherException( 'Invalid ItemId diff: ' . $diffOp->getType() ); } - throw new PatcherException( 'Invalid ItemId diff' ); + Assert::postcondition( $id instanceof ItemId, 'DiffOp must yield an ItemId' ); + return $id; } } diff --git a/tests/phpunit/composer/DataModel/Services/Diff/LexemeDiffTest.php b/tests/phpunit/composer/DataModel/Services/Diff/LexemeDiffTest.php new file mode 100644 index 0000000..2ed92e1 --- /dev/null +++ b/tests/phpunit/composer/DataModel/Services/Diff/LexemeDiffTest.php @@ -0,0 +1,82 @@ +<?php + +namespace Wikibase\Lexeme\Tests\DataModel\Services\Diff; + +use Diff\DiffOp\Diff\Diff; +use Diff\DiffOp\DiffOpAdd; +use Diff\DiffOp\DiffOpChange; +use Diff\DiffOp\DiffOpRemove; +use Wikibase\DataModel\Entity\ItemId; +use Wikibase\DataModel\Entity\PropertyId; +use Wikibase\DataModel\Services\Diff\StatementListDiffer; +use Wikibase\DataModel\Snak\PropertyNoValueSnak; +use Wikibase\DataModel\Statement\Statement; +use Wikibase\DataModel\Statement\StatementList; +use Wikibase\Lexeme\DataModel\Services\Diff\LexemeDiff; + +/** + * @covers Wikibase\Lexeme\DataModel\Services\Diff\LexemeDiff + * + * @license GPL-2.0+ + */ +class LexemeDiffTest extends \PHPUnit_Framework_TestCase { + + public function testGetLemmaDiff() { + $diff = new LexemeDiff( [] ); + $this->assertEmpty( $diff->getLemmasDiff(), 'lemma diffs' ); + + $lemmasDiff = new Diff( [ + 'en' => new DiffOpAdd( 'foobar' ), + 'de' => new DiffOpRemove( 'onoez' ), + 'nl' => new DiffOpChange( 'foo', 'bar' ), + ], true ); + + $diff = new LexemeDiff( [ 'lemmas' => $lemmasDiff ] ); + $this->assertNotEmpty( $diff->getLemmasDiff(), 'lemmas diff' ); + } + + public function testGetType() { + $diff = new LexemeDiff(); + $this->assertSame( 'diff/lexeme', $diff->getType() ); + } + + public function testIsEmpty_true() { + $diff = new LexemeDiff(); + $this->assertTrue( $diff->isEmpty() ); + } + + public function provideNonEmptyDiffOpLists() { + $q1 = new ItemId( 'Q1' ); + $q2 = new ItemId( 'Q2' ); + + $lemmaDiff = new Diff( [ + 'en' => new DiffOpAdd( 'foobar' ), + 'de' => new DiffOpRemove( 'onoez' ), + 'nl' => new DiffOpChange( 'foo', 'bar' ), + ], true ); + + $statement = new Statement( new PropertyNoValueSnak( new PropertyId( 'P42' ) ) ); + $statement->setGuid( 'EntityDiffTest$foo' ); + + $statementListDiffer = new StatementListDiffer(); + $claimDiff = $statementListDiffer->getDiff( + new StatementList( $statement ), + new StatementList() + ); + + return [ + 'lemmas' => [ new LexemeDiff( [ 'lemmas' => $lemmaDiff ] ) ], + 'claim' => [ new LexemeDiff( [ 'claim' => $claimDiff ] ) ], + 'language' => [ new LexemeDiff( [ 'language' => new DiffOpChange( $q1, $q2 ) ] ) ], + 'lexicalCategory' => [ new LexemeDiff( [ 'lexicalCategory' => new DiffOpChange( $q1, $q2 ) ] ) ], + ]; + } + + /** + * @dataProvider provideNonEmptyDiffOpLists + */ + public function testIsEmpty_false( LexemeDiff $diff ) { + $this->assertFalse( $diff->isEmpty() ); + } + +} diff --git a/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferTest.php b/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferTest.php index 1714ac3..4f69c63 100644 --- a/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferTest.php +++ b/tests/phpunit/composer/DataModel/Services/Diff/LexemeDifferTest.php @@ -2,6 +2,7 @@ namespace Wikibase\Lexeme\Tests\DataModel\Services\Diff; +use Diff\DiffOp\DiffOpAdd; use PHPUnit_Framework_TestCase; use Wikibase\DataModel\Entity\Item; use Wikibase\DataModel\Entity\ItemId; @@ -69,8 +70,8 @@ $differ = new LexemeDiffer(); $diff = $differ->diffLexemes( $firstLexeme, $secondLexeme ); - /** @var LexemeDiff $diff */ - $this->assertCount( 1, $diff->getLexicalCategoryDiff()->getAdditions() ); + $this->assertTrue( isset( $diff['lexicalCategory'] ), '$diff[\'lexicalCategory\']' ); + $this->assertInstanceOf( DiffOpAdd::class, $diff['lexicalCategory'] ); } public function testLanguageIsDiffed() { @@ -82,8 +83,8 @@ $differ = new LexemeDiffer(); $diff = $differ->diffLexemes( $firstLexeme, $secondLexeme ); - /** @var LexemeDiff $diff */ - $this->assertCount( 1, $diff->getLanguageDiff()->getAdditions() ); + $this->assertTrue( isset( $diff['language'] ), '$diff[\'language\']' ); + $this->assertInstanceOf( DiffOpAdd::class, $diff['language'] ); } public function testGivenEmptyLexeme_constructionDiffIsEmpty() { diff --git a/tests/phpunit/composer/DataModel/Services/Diff/LexemePatcherTest.php b/tests/phpunit/composer/DataModel/Services/Diff/LexemePatcherTest.php index 13f019a..2fb3e0e 100644 --- a/tests/phpunit/composer/DataModel/Services/Diff/LexemePatcherTest.php +++ b/tests/phpunit/composer/DataModel/Services/Diff/LexemePatcherTest.php @@ -108,9 +108,7 @@ $lexeme->setLexicalCategory( $removedLexicalCategory ); $patch = new LexemeDiff( [ - 'lexicalCategory' => new Diff( [ - 'id' => new DiffOpChange( 'Q11', 'Q22' ), - ] ), + 'lexicalCategory' => new DiffOpChange( $removedLexicalCategory, $addedLexicalCategory ), ] ); $expected = new Lexeme(); @@ -130,9 +128,7 @@ $lexeme->setLanguage( $removedLanguage ); $patch = new LexemeDiff( [ - 'language' => new Diff( [ - 'id' => new DiffOpChange( 'Q1', 'Q2' ), - ] ), + 'language' => new DiffOpChange( $removedLanguage, $addedLanguage ), ] ); $expected = new Lexeme(); -- To view, visit https://gerrit.wikimedia.org/r/347621 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If46efb61a0abacc7632b125643eb749946143785 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/WikibaseLexeme Gerrit-Branch: master Gerrit-Owner: Daniel Kinzler <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
