jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/351628 )
Change subject: Parse and correctly compare time values
......................................................................
Parse and correctly compare time values
RangeCheckerHelper’s methods are replaced with two methods that more
closely reflect what the range checkers actually require: comparison of
two values (RangeChecker), and the difference between two values
(DiffWithinRangeChecker). For time values, there is no representation
that getComparativeValue() could have returned that would also let you
calculate a difference.
Two more new methods, parseTime() and parseQuantity(), parse time and
quantity values. RangeChecker uses these methods to parse its min/max
parameters before passing them to getComparison(), which fixes T164279.
parseTime() uses Wikibase’s standard time parser chain; parseQuantity()
is currently very simple because only simple quantities are recorded in
template parameters right now, but might need unit support later.
The test skipped in 1644986 can be unskipped again, since the parameters
are now properly parsed, and the test added in 7644ede can now use a
zero-padded timestamp like all other timestamps in the file. Two new
tests for RangeChecker are also added, which test that parameters
containing only a year or only a date work correctly, and two new tests
for RangeCheckerHelper test the new function. The tests for
RangeCheckerHelper are completely replaced to test the new methods.
Bug: T164279
Change-Id: I902156e4e013e6c5f3c9c73bf35919de9264650a
---
M includes/ConstraintCheck/Checker/DiffWithinRangeChecker.php
M includes/ConstraintCheck/Checker/RangeChecker.php
M includes/ConstraintCheck/Helper/RangeCheckerHelper.php
M tests/phpunit/Checker/RangeChecker/RangeCheckerHelperTest.php
M tests/phpunit/Checker/RangeChecker/RangeCheckerTest.php
5 files changed, 231 insertions(+), 34 deletions(-)
Approvals:
jenkins-bot: Verified
Thiemo Mättig (WMDE): Looks good to me, approved
diff --git a/includes/ConstraintCheck/Checker/DiffWithinRangeChecker.php
b/includes/ConstraintCheck/Checker/DiffWithinRangeChecker.php
index 3f39227..3b2aee6 100644
--- a/includes/ConstraintCheck/Checker/DiffWithinRangeChecker.php
+++ b/includes/ConstraintCheck/Checker/DiffWithinRangeChecker.php
@@ -94,8 +94,6 @@
return new CheckResult( $entity->getId(), $statement,
$constraint->getConstraintTypeQid(), $constraint->getConstraintId(),
$parameters, CheckResult::STATUS_VIOLATION, $message );
}
- $thisValue = $this->rangeCheckerHelper->getComparativeValue(
$dataValue );
-
// checks only the first occurrence of the referenced property
(this constraint implies a single value constraint on that property)
/** @var Statement $statement */
foreach ( $entity->getStatements() as $statement ) {
@@ -112,10 +110,7 @@
}
if ( $mainSnak->getDataValue()->getType() ===
$dataValue->getType() && $mainSnak->getType() === 'value' ) {
- $thatValue =
$this->rangeCheckerHelper->getComparativeValue( $mainSnak->getDataValue() );
-
- // negative difference is possible
- $diff = $thisValue - $thatValue;
+ $diff =
$this->rangeCheckerHelper->getDifference( $dataValue, $mainSnak->getDataValue()
);
if ( $diff < $min || $diff > $max ) {
$message = wfMessage(
"wbqc-violation-message-diff-within-range" )->escaped();
diff --git a/includes/ConstraintCheck/Checker/RangeChecker.php
b/includes/ConstraintCheck/Checker/RangeChecker.php
index 560d21a..f5d07f5 100644
--- a/includes/ConstraintCheck/Checker/RangeChecker.php
+++ b/includes/ConstraintCheck/Checker/RangeChecker.php
@@ -83,6 +83,8 @@
$max =
$constraintParameters['maximum_quantity'];
$parameters['minimum_quantity'] =
$this->constraintParameterParser->parseSingleParameter( $min, true );
$parameters['maximum_quantity'] =
$this->constraintParameterParser->parseSingleParameter( $max, true );
+ $min =
$this->rangeCheckerHelper->parseQuantity( $min );
+ $max =
$this->rangeCheckerHelper->parseQuantity( $max );
} else {
$message = wfMessage(
'wbqc-violation-message-range-parameters-needed' )
->params( 'quantity',
'minimum_quantity', 'maximum_quantity' )->escaped();
@@ -122,6 +124,8 @@
if ( $max === 'now' ) {
$max = $now;
}
+ $min = $this->rangeCheckerHelper->parseTime(
$min );
+ $max = $this->rangeCheckerHelper->parseTime(
$max );
}
} else {
$message = wfMessage(
'wbqc-violation-message-value-needed-of-types-2' )
@@ -140,9 +144,8 @@
);
}
- $comparativeValue =
$this->rangeCheckerHelper->getComparativeValue( $dataValue );
-
- if ( $comparativeValue < $min || $comparativeValue > $max ) {
+ if ( $this->rangeCheckerHelper->getComparison( $min, $dataValue
) > 0 ||
+ $this->rangeCheckerHelper->getComparison( $dataValue,
$max ) > 0 ) {
$message = wfMessage( "wbqc-violation-message-range"
)->escaped();
$status = CheckResult::STATUS_VIOLATION;
} else {
diff --git a/includes/ConstraintCheck/Helper/RangeCheckerHelper.php
b/includes/ConstraintCheck/Helper/RangeCheckerHelper.php
index 00338e7..f4bbfea 100644
--- a/includes/ConstraintCheck/Helper/RangeCheckerHelper.php
+++ b/includes/ConstraintCheck/Helper/RangeCheckerHelper.php
@@ -5,8 +5,11 @@
use DataValues\DataValue;
use DataValues\QuantityValue;
use DataValues\TimeValue;
+use DataValues\TimeValueCalculator;
use DataValues\UnboundedQuantityValue;
use InvalidArgumentException;
+use ValueParsers\ValueParser;
+use Wikibase\Repo\Parsers\TimeParserFactory;
/**
* Class for helper functions for range checkers.
@@ -17,16 +20,109 @@
*/
class RangeCheckerHelper {
- public function getComparativeValue( DataValue $dataValue ) {
- if ( $dataValue instanceof TimeValue ) {
- return $dataValue->getTime();
- } elseif ( $dataValue instanceof QuantityValue ) {
- return $dataValue->getAmount()->getValue();
- } elseif ( $dataValue instanceof UnboundedQuantityValue ) {
- return $dataValue->getAmount()->getValue();
+ /**
+ * @var ValueParser
+ */
+ private $timeParser;
+
+ /**
+ * @var TimeValueCalculator
+ */
+ private $timeCalculator;
+
+ public function __construct() {
+ $this->timeParser = (new TimeParserFactory())->getTimeParser();
+ $this->timeCalculator = new TimeValueCalculator();
+ }
+
+ /**
+ * @param DataValue $lhs left-hand side
+ * @param DataValue $rhs right-hand side
+ *
+ * @throws InvalidArgumentException if the values do not both have the
same, supported data value type
+ * @return integer An integer less then, equal to, or greater than zero
+ * when $lhs is respectively less than, equal to, or
greater than $rhs.
+ * (In other words, just like the “spaceship” operator
<=>.)
+ */
+ public function getComparison( DataValue $lhs, DataValue $rhs ) {
+ if ( $lhs->getType() === 'time' && $rhs->getType() === 'time' )
{
+ $lhsTimestamp = $this->timeCalculator->getTimestamp(
$lhs );
+ $rhsTimestamp = $this->timeCalculator->getTimestamp(
$rhs );
+ if ( $lhsTimestamp < $rhsTimestamp ) {
+ return -1;
+ } elseif ( $lhsTimestamp > $rhsTimestamp ) {
+ return 1;
+ } else {
+ return 0;
+ }
+ }
+ if ( $lhs->getType() === 'quantity' && $rhs->getType() ===
'quantity' ) {
+ // TODO normalize values: T164371
+ $lhsValue = $lhs->getAmount()->getValue();
+ $rhsValue = $rhs->getAmount()->getValue();
+ if ( $lhsValue < $rhsValue ) {
+ return -1;
+ } elseif ( $lhsValue > $rhsValue ) {
+ return 1;
+ } else {
+ return 0;
+ }
}
- throw new InvalidArgumentException( 'Unsupported data value
type' );
+ throw new InvalidArgumentException( 'Unsupported or different
data value types' );
+ }
+
+ /**
+ * Computes $minuend - $subtrahend, in a format depending on the data
type.
+ * For time values, the difference is in years;
+ * otherwise, the difference is simply the numerical difference between
the quantities.
+ * (The units of the quantities are currently ignored: see T164371.)
+ *
+ * @param TimeValue|QuantityValue|UnboundedQuantityValue $minuend
+ * @param TimeValue|QuantityValue|UnboundedQuantityValue $subtrahend
+ *
+ * @throws InvalidArgumentException if the values do not both have the
same, supported data value type
+ * @return float
+ */
+ public function getDifference( DataValue $minuend, DataValue
$subtrahend ) {
+ if ( $minuend->getType() === 'time' && $subtrahend->getType()
=== 'time' ) {
+ // difference in years
+ if ( !preg_match( '/^([-+]\d{1,16})-/',
$minuend->getTime(), $minuendMatches ) ||
+ !preg_match( '/^([-+]\d{1,16})-/',
$subtrahend->getTime(), $subtrahendMatches ) ) {
+ throw new InvalidArgumentException(
'TimeValue::getTime() did not match expected format' );
+ }
+ $minuendYear = (float)$minuendMatches[1];
+ $subtrahendYear = (float)$subtrahendMatches[1];
+ $diff = $minuendYear - $subtrahendYear;
+ if ( $minuendYear > 0.0 && $subtrahendYear < 0.0 ) {
+ $diff -= 1.0; // there is no year 0, remove it
from difference
+ } elseif ( $minuendYear < 0.0 && $subtrahendYear > 0.0)
{
+ $diff += 1.0; // there is no year 0, remove it
from negative difference
+ }
+ return $diff;
+ }
+ if ( $minuend->getType() === 'quantity' &&
$subtrahend->getType() === 'quantity' ) {
+ // TODO normalize values: T164371
+ return (float)$minuend->getAmount()->getValue() -
(float)$subtrahend->getAmount()->getValue();
+ }
+
+ throw new InvalidArgumentException( 'Unsupported or different
data value types' );
+ }
+
+ /**
+ * @param string $timeString
+ * @return TimeValue
+ */
+ public function parseTime( $timeString ) {
+ return $this->timeParser->parse( $timeString );
+ }
+
+ /**
+ * @param string $quantityString
+ * @return QuantityValue|UnboundedQuantityValue
+ */
+ public function parseQuantity( $quantityString ) {
+ return UnboundedQuantityValue::newFromNumber( $quantityString );
}
}
diff --git a/tests/phpunit/Checker/RangeChecker/RangeCheckerHelperTest.php
b/tests/phpunit/Checker/RangeChecker/RangeCheckerHelperTest.php
index 02139e8..705af1b 100644
--- a/tests/phpunit/Checker/RangeChecker/RangeCheckerHelperTest.php
+++ b/tests/phpunit/Checker/RangeChecker/RangeCheckerHelperTest.php
@@ -26,20 +26,62 @@
class RangeCheckerHelperTest extends PHPUnit_Framework_TestCase {
/**
- * @dataProvider getComparativeValueProvider
+ * @dataProvider getComparisonProvider
*/
- public function testGetComparativeValue( $expected, DataValue
$dataValue ) {
+ public function testGetComparison( $expected, DataValue $lhs, DataValue
$rhs ) {
+ $this->assertContains( $expected, [ -1, 0, 1 ], '$expected must
be -1, 0, or 1' );
$rangeCheckHelper = new RangeCheckerHelper();
- $comparativeValue = $rangeCheckHelper->getComparativeValue(
$dataValue );
-
- $this->assertSame( $expected, $comparativeValue );
+ $actual = $rangeCheckHelper->getComparison( $lhs, $rhs );
+ switch ( $expected ) {
+ case -1:
+ $this->assertLessThan( 0, $actual );
+ break;
+ case 0:
+ $this->assertSame( 0, $actual );
+ break;
+ case 1:
+ $this->assertGreaterThan( 0, $actual );
+ break;
+ }
}
- public function getComparativeValueProvider() {
+ public function getComparisonProvider() {
$cases = [
- [ '+1970-01-01T00:00:00Z', $this->getTimeValue() ],
- [ '+42', $this->getQuantityValue() ],
- [ '+9000', UnboundedQuantityValue::newFromNumber(
'+9000' ) ]
+ [ -1, $this->getTimeValue( 1970 ), $this->getTimeValue(
1971 ) ],
+ [ 0, $this->getTimeValue( 1970 ), $this->getTimeValue(
1970 ) ],
+ [ 1, $this->getTimeValue( 1971 ), $this->getTimeValue(
1970 ) ],
+ [ -1, $this->getQuantityValue( 42.0 ),
$this->getQuantityValue( 1337.0 ) ],
+ [ 0, $this->getQuantityValue( 42.0 ),
$this->getQuantityValue( 42.0 ) ],
+ [ 1, $this->getQuantityValue( 1337.0 ),
$this->getQuantityValue( 42.0 ) ],
+ [ -1, $this->getUnboundedQuantityValue( 42.0 ),
$this->getUnboundedQuantityValue( 1337.0 ) ],
+ [ 0, $this->getUnboundedQuantityValue( 42.0 ),
$this->getUnboundedQuantityValue( 42.0 ) ],
+ [ 1, $this->getUnboundedQuantityValue( 1337.0 ),
$this->getUnboundedQuantityValue( 42.0 ) ],
+ ];
+
+ return $cases;
+ }
+
+ /**
+ * @dataProvider getDifferenceProvider
+ */
+ public function testGetDifference( $expected, DataValue $minuend,
DataValue $subtrahend ) {
+ $rangeCheckHelper = new RangeCheckerHelper();
+ $actual = $rangeCheckHelper->getDifference( $minuend,
$subtrahend );
+ $this->assertSame( $expected, $actual );
+ }
+
+ public function getDifferenceProvider() {
+ $cases = [
+ [ -1.0, $this->getTimeValue( 1970 ),
$this->getTimeValue( 1971 ) ],
+ [ 1.0, $this->getTimeValue( 1971 ),
$this->getTimeValue( 1970 ) ],
+ [ -1.0, $this->getTimeValue( -1 ), $this->getTimeValue(
1 ) ],
+ [ 1.0, $this->getTimeValue( 1 ), $this->getTimeValue(
-1 ) ],
+ [ -1.0, $this->getTimeValue( -1971 ),
$this->getTimeValue( -1970 ) ],
+ [ 1.0, $this->getTimeValue( -1970 ),
$this->getTimeValue( -1971 ) ],
+ [ -1295.0, $this->getQuantityValue( 42.0 ),
$this->getQuantityValue( 1337.0 ) ],
+ [ 1295.0, $this->getQuantityValue( 1337.0 ),
$this->getQuantityValue( 42.0 ) ],
+ [ -1295.0, $this->getQuantityValue( 42.0 ),
$this->getUnboundedQuantityValue( 1337.0 ) ],
+ [ -1295.0, $this->getUnboundedQuantityValue( 42.0 ),
$this->getQuantityValue( 1337.0 ) ]
];
return $cases;
@@ -48,14 +90,38 @@
/**
* @expectedException InvalidArgumentException
*/
- public function
testGetComparativeValue_unsupportedDataValueTypeThrowsException() {
+ public function
testGetComparison_unsupportedDataValueTypeThrowsException() {
$rangeCheckHelper = new RangeCheckerHelper();
- $rangeCheckHelper->getComparativeValue( new StringValue(
'kittens' ) );
+ $rangeCheckHelper->getComparison( new StringValue( 'kittens' ),
new StringValue( 'puppies' ) );
}
- private function getTimeValue() {
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function
testGetComparison_differingDataValueTypeThrowsException() {
+ $rangeCheckHelper = new RangeCheckerHelper();
+ $rangeCheckHelper->getComparison( $this->getQuantityValue( 42.0
), $this->getTimeValue( 1970 ) );
+ }
+
+ public function testParseTime_year() {
+ $rangeCheckHelper = new RangeCheckerHelper();
+ $this->assertSame( '+1970-00-00T00:00:00Z',
$rangeCheckHelper->parseTime( '1970' )->getTime() );
+ }
+
+ public function testParseTime_yearMonthDay() {
+ $rangeCheckHelper = new RangeCheckerHelper();
+ $this->assertSame( '+1970-01-01T00:00:00Z',
$rangeCheckHelper->parseTime( '1970-01-01' )->getTime() );
+ }
+
+ /**
+ * @param integer $year
+ *
+ * @return TimeValue
+ */
+ private function getTimeValue( $year ) {
+ $yearString = $year > 0 ? "+$year" : "$year";
return new TimeValue(
- '+00000001970-01-01T00:00:00Z',
+ "$yearString-01-01T00:00:00Z",
0,
0,
0,
@@ -64,10 +130,24 @@
);
}
- private function getQuantityValue() {
- $decimalValue = new DecimalValue( 42 );
+ /**
+ * @param float $amount
+ *
+ * @return QuantityValue
+ */
+ private function getQuantityValue( $amount ) {
+ $decimalValue = new DecimalValue( $amount );
return new QuantityValue( $decimalValue, '1', $decimalValue,
$decimalValue );
}
+ /**
+ * @param float $amount
+ *
+ * @return UnboundedQuantityValue
+ */
+ private function getUnboundedQuantityValue( $amount ) {
+ return UnboundedQuantityValue::newFromNumber( $amount );
+ }
+
}
diff --git a/tests/phpunit/Checker/RangeChecker/RangeCheckerTest.php
b/tests/phpunit/Checker/RangeChecker/RangeCheckerTest.php
index 72aff88..eb678a1 100644
--- a/tests/phpunit/Checker/RangeChecker/RangeCheckerTest.php
+++ b/tests/phpunit/Checker/RangeChecker/RangeCheckerTest.php
@@ -102,7 +102,6 @@
}
public function testRangeConstraintTimeWithinRange() {
- $this->markTestSkipped( 'RangeChecker does not correctly parse
the parameters to compare – see T164279.' );
$min = '+00000001960-01-01T00:00:00Z';
$max = '+00000001980-01-01T00:00:00Z';
$statement = new Statement( new PropertyValueSnak( new
PropertyId( 'P1457' ), $this->timeValue ) );
@@ -115,7 +114,7 @@
}
public function testRangeConstraintTimeWithinRangeToNow() {
- $min = '+1960-01-01T00:00:00Z'; // TODO add leading zeroes as
in testRangeConstraintTimeWithinRange
+ $min = '+00000001960-01-01T00:00:00Z';
$max = 'now';
$statement = new Statement( new PropertyValueSnak( new
PropertyId( 'P1457' ), $this->timeValue ) );
$constraintParameters = array(
@@ -126,6 +125,30 @@
$this->assertEquals( 'compliance', $checkResult->getStatus(),
'check should comply' );
}
+ public function testRangeConstraintTimeWithinYearRange() {
+ $min = '1960';
+ $max = '1980';
+ $statement = new Statement( new PropertyValueSnak( new
PropertyId( 'P1457' ), $this->timeValue ) );
+ $constraintParameters = array(
+ 'minimum_date' => $min,
+ 'maximum_date' => $max
+ );
+ $checkResult = $this->checker->checkConstraint( $statement,
$this->getConstraintMock( $constraintParameters ), $this->getEntity() );
+ $this->assertEquals( 'compliance', $checkResult->getStatus(),
'check should comply' );
+ }
+
+ public function testRangeConstraintTimeWithinYearMonthDayRange() {
+ $min = '1969-12-31';
+ $max = '1970-01-02';
+ $statement = new Statement( new PropertyValueSnak( new
PropertyId( 'P1457' ), $this->timeValue ) );
+ $constraintParameters = array(
+ 'minimum_date' => $min,
+ 'maximum_date' => $max
+ );
+ $checkResult = $this->checker->checkConstraint( $statement,
$this->getConstraintMock( $constraintParameters ), $this->getEntity() );
+ $this->assertEquals( 'compliance', $checkResult->getStatus(),
'check should comply' );
+ }
+
public function testRangeConstraintTimeTooSmall() {
$min = '+00000001975-01-01T00:00:00Z';
$max = '+00000001980-01-01T00:00:00Z';
--
To view, visit https://gerrit.wikimedia.org/r/351628
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I902156e4e013e6c5f3c9c73bf35919de9264650a
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/WikibaseQualityConstraints
Gerrit-Branch: master
Gerrit-Owner: Lucas Werkmeister (WMDE) <[email protected]>
Gerrit-Reviewer: Jonas Kress (WMDE) <[email protected]>
Gerrit-Reviewer: Lucas Werkmeister (WMDE) <[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
