Mailing List Archive

jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/403969 )

Change subject: Fix mistakes and refactor FormDiffView and related
......................................................................


Fix mistakes and refactor FormDiffView and related

I believe this is all very much trivial refactoring. This patch also
fixes a few mistakes, but these happened in comments.

Bug: T182424
Change-Id: I4e6b9531d56405e8d2c72d501c322ab71e6ead29
---
M src/Diff/FormDiffView.php
M src/Diff/LexemeDiffVisualizer.php
M tests/phpunit/mediawiki/Diff/FormDiffViewTest.php
3 files changed, 54 insertions(+), 60 deletions(-)

Approvals:
Ladsgroup: Looks good to me, approved
jenkins-bot: Verified



diff --git a/src/Diff/FormDiffView.php b/src/Diff/FormDiffView.php
index 1d82223..38c46e8 100644
--- a/src/Diff/FormDiffView.php
+++ b/src/Diff/FormDiffView.php
@@ -22,12 +22,12 @@
class FormDiffView extends BasicDiffView {

/**
- * @var ClaimDiffer|null
+ * @var ClaimDiffer
*/
private $claimDiffer;

/**
- * @var ClaimDifferenceVisualizer|null
+ * @var ClaimDifferenceVisualizer
*/
private $claimDiffVisualizer;

@@ -61,26 +61,29 @@
* @param string[] $path
* @param DiffOp $op
*
- * @return string
- * @throws MWException
+ * @return string HTML
*/
protected function generateOpHtml( array $path, DiffOp $op ) {
- $html = '';
if ( $op->isAtomic() ) {
return parent::generateOpHtml( $path, $op );
}
+
+ $html = '';
+
foreach ( $op as $key => $subOp ) {
- if ( !$subOp instanceof ChangeFormDiffOp ) {
- $html .= $this->generateOpHtml( array_merge( $path, [ $key ] ), $subOp );
- } else {
+ if ( $subOp instanceof ChangeFormDiffOp ) {
$html .= $this->generateFormOpHtml( $path, $subOp, $key );
+ } else {
+ $html .= $this->generateOpHtml( array_merge( $path, [ $key ] ), $subOp );
}
}
+
return $html;
}

- private function generateFormOpHtml( $path, ChangeFormDiffOp $op, $key ) {
+ private function generateFormOpHtml( array $path, ChangeFormDiffOp $op, $key ) {
$html = '';
+
foreach ( $op->getStatementsDiffOps() as $claimDiffOp ) {
$html .= $this->getClaimDiffHtml(
$claimDiffOp,
@@ -108,33 +111,31 @@
}

/**
- * @param DiffOp $claimDiffOp
+ * @param DiffOp $diffOp
*
* @return string HTML
* @throws MWException
*/
- protected function getClaimDiffHtml( DiffOp $claimDiffOp, array $path ) {
- if ( $claimDiffOp instanceof DiffOpChange ) {
- $claimDifference = $this->claimDiffer->diffClaims(
- $claimDiffOp->getOldValue(),
- $claimDiffOp->getNewValue()
- );
- return $this->claimDiffVisualizer->visualizeClaimChange(
- $claimDifference,
- $claimDiffOp->getNewValue(),
- $path
- );
- }
+ private function getClaimDiffHtml( DiffOp $diffOp, array $path ) {
+ switch ( true ) {
+ case $diffOp instanceof DiffOpChange:
+ return $this->claimDiffVisualizer->visualizeClaimChange(
+ $this->claimDiffer->diffClaims(
+ $diffOp->getOldValue(),
+ $diffOp->getNewValue()
+ ),
+ $diffOp->getNewValue(),
+ $path
+ );

- if ( $claimDiffOp instanceof DiffOpAdd ) {
- return $this->claimDiffVisualizer->visualizeNewClaim( $claimDiffOp->getNewValue(), $path );
- } elseif ( $claimDiffOp instanceof DiffOpRemove ) {
- return $this->claimDiffVisualizer->visualizeRemovedClaim(
- $claimDiffOp->getOldValue(),
- $path
- );
- } else {
- throw new MWException( 'Encountered an unexpected diff operation type for a claim' );
+ case $diffOp instanceof DiffOpAdd:
+ return $this->claimDiffVisualizer->visualizeNewClaim( $diffOp->getNewValue(), $path );
+
+ case $diffOp instanceof DiffOpRemove:
+ return $this->claimDiffVisualizer->visualizeRemovedClaim( $diffOp->getOldValue(), $path );
+
+ default:
+ throw new MWException( 'Encountered an unexpected diff operation type for a claim' );
}
}

diff --git a/src/Diff/LexemeDiffVisualizer.php b/src/Diff/LexemeDiffVisualizer.php
index a8210f2..7c4c610 100644
--- a/src/Diff/LexemeDiffVisualizer.php
+++ b/src/Diff/LexemeDiffVisualizer.php
@@ -4,7 +4,7 @@

use Diff\DiffOp\Diff\Diff;
use MessageLocalizer;
-use Wikibase\DataModel\Services\Diff\EntityDiff;
+use Wikibase\Lexeme\DataModel\Services\Diff\LexemeDiff;
use Wikibase\Repo\Content\EntityContentDiff;
use Wikibase\Repo\Diff\BasicDiffView;
use Wikibase\Repo\Diff\BasicEntityDiffVisualizer;
@@ -13,8 +13,6 @@
use Wikibase\Repo\Diff\EntityDiffVisualizer;

/**
- * Class for generating views of EntityDiff objects.
- *
* @license GPL-2.0+
* @author Amir Sarabadani
*/
@@ -31,12 +29,12 @@
private $basicEntityDiffVisualizer;

/**
- * @var ClaimDiffer|null
+ * @var ClaimDiffer
*/
private $claimDiffer;

/**
- * @var ClaimDifferenceVisualizer|null
+ * @var ClaimDifferenceVisualizer
*/
private $claimDiffVisualizer;

@@ -53,31 +51,26 @@
}

/**
- * Generates and returns an HTML visualization of the provided EntityContentDiff.
- *
* @param EntityContentDiff $diff
*
- * @return string
+ * @return string HTML
*/
public function visualizeEntityContentDiff( EntityContentDiff $diff ) {
if ( $diff->isEmpty() ) {
return '';
}

- $basicHtml = $this->basicEntityDiffVisualizer->visualizeEntityContentDiff( $diff );
-
- return $basicHtml . $this->visualizeEntityDiff( $diff->getEntityDiff() );
+ return $this->basicEntityDiffVisualizer->visualizeEntityContentDiff( $diff )
+ . $this->visualizeEntityDiff( $diff->getEntityDiff() );
}

/**
- * Generates and returns an HTML visualization of the provided EntityDiff.
+ * @param LexemeDiff $diff
*
- * @param EntityDiff $diff
- *
- * @return string
+ * @return string HTML
*/
- private function visualizeEntityDiff( EntityDiff $diff ) {
- $basicDiff = ( new BasicDiffView(
+ private function visualizeEntityDiff( LexemeDiff $diff ) {
+ $basicDiffView = new BasicDiffView(
[],
new Diff(
[
@@ -90,8 +83,9 @@
],
true
)
- ) )->getHtml();
- $formDiff = ( new FormDiffView(
+ );
+
+ $formDiffView = new FormDiffView(
[],
new Diff(
[
@@ -103,8 +97,9 @@
$this->claimDiffer,
$this->claimDiffVisualizer,
$this->messageLocalizer
- ) )->getHtml();
- return $basicDiff . $formDiff;
+ );
+
+ return $basicDiffView->getHtml() . $formDiffView->getHtml();
}

}
diff --git a/tests/phpunit/mediawiki/Diff/FormDiffViewTest.php b/tests/phpunit/mediawiki/Diff/FormDiffViewTest.php
index e4e3634..13ee9c4 100644
--- a/tests/phpunit/mediawiki/Diff/FormDiffViewTest.php
+++ b/tests/phpunit/mediawiki/Diff/FormDiffViewTest.php
@@ -3,8 +3,8 @@
namespace Wikibase\Lexeme\Tests\Diff;

use Diff\Comparer\ComparableComparer;
+use Diff\Differ\OrderedListDiffer;
use Diff\DiffOp\Diff\Diff;
-use Diff\OrderedListDiffer;
use MessageLocalizer;
use RawMessage;
use Wikibase\DataModel\Entity\PropertyId;
@@ -36,18 +36,14 @@

/**
* @param string $returnValue
- * @param string $format
*
* @return SnakFormatter
*/
- public function newSnakFormatter(
- $returnValue = '<i>SNAK</i>',
- $format = SnakFormatter::FORMAT_HTML
- ) {
+ public function newSnakFormatter( $returnValue = '<i>SNAK</i>' ) {
$instance = $this->getMock( SnakFormatter::class );
$instance->expects( $this->any() )
->method( 'getFormat' )
- ->will( $this->returnValue( $format ) );
+ ->will( $this->returnValue( SnakFormatter::FORMAT_HTML ) );
$instance->expects( $this->any() )
->method( 'canFormatSnak' )
->will( $this->returnValue( true ) );
@@ -100,6 +96,8 @@
}

/**
+ * @param ChangeFormDiffOp $diff
+ *
* @return FormDiffView
*/
private function getDiffView( ChangeFormDiffOp $diff ) {
@@ -225,7 +223,7 @@
}

/**
- * @return mixed
+ * @return Statement
*/
private function someStatement( $propertyId, $guid ) {
$statement = new Statement(

--
To view, visit https://gerrit.wikimedia.org/r/403969
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4e6b9531d56405e8d2c72d501c322ab71e6ead29
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/WikibaseLexeme
Gerrit-Branch: master
Gerrit-Owner: Thiemo Kreuz (WMDE) <thiemo.kreuz@wikimedia.de>
Gerrit-Reviewer: Ladsgroup <Ladsgroup@gmail.com>
Gerrit-Reviewer: WMDE-leszek <leszek.manicki@wikimedia.de>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits