Add getLowerTimestamp() and getHigherTimestamp() #140

davidabian · 2018-06-25T14:59:36Z

Add public functions getLowerTimestamp() and getHigherTimestamp() together with their tests and private functions. They will be used in WikibaseQualityConstraints to fix https://phabricator.wikimedia.org/T195226.

Add functions getLowerTimestamp() and getHigherTimestamp(). They'll be used in WikibaseQualityConstraints. PS: I've never programmed in PHP nor I'm familiar with this repository, so please feel free to tell me what I may be doing wrong.

lucaswerkmeister · 2018-06-25T15:19:22Z

Can you add some tests for this in TimeValueCalculatorTest.php? I haven’t looked at this in detail yet, but I doubt we’d merge this without tests anyways, and they’ll also make it easier to understand the patch and review it :)

JeroenDeDauw · 2018-06-25T16:23:45Z

This does not follow the WMDE code style which is used in this repo. If you use PhpStorm, then you can find the definition of the style here: https://github.com/wmde/intellij-settings. If not, perhaps someone can run their own PhpStorm formatting on the code for you.

davidabian · 2018-06-25T17:09:13Z

Thanks!

davidabian · 2018-07-02T13:35:34Z

Please let's not forget this proposal, :-) we need it for https://phabricator.wikimedia.org/T195226, and I need to resolve that task to start https://phabricator.wikimedia.org/T141859.

What changes should I make now? Ping @thiemowmde.

thiemowmde · 2018-07-02T15:34:10Z

Can you please link the relevant Phabricator tickets in the commit message, as well as link from the relevant Phabricator tickets to this patch?

As far as I can see this patch contains a lot of changes that are unrelated to what the commit message talks about:

Please do reformatting of unrelated code in a separate patch. This helps a lot in reviewing the two independently.
Do the constants TIMESTAMP_ZERO and HIGHEST_TIMESTAMP need to be public?
Technically the latest possible timestamp is +9999999999999999-12-31T23:59:61Z, as we allow up to two leap seconds.
What was wrong with the documentation of getTimestamp? As far as I can see all you did was adding a newline and reflowing the paragraph. Why?
Please replace strcmp( substr( $timestamp, 0, 1 ), '-' ) === 0 with something that reads better, e.g. $timestamp[0] === '-'.

I find the "WARNING: Day 31 will be applied to all months" problematic. What it essentially means is that this code is not yet ready, as it will calculate bad timestamps that don't reflect the precision properly. For example, a timestamp at 2018-03-31 with a range of 1 month will probably calculate a range that starts at – I think – 2018-02-31. PHP converts this to 2018-03-04, which is not 1 month before the original date.

As I don't know where exactly this code is supposed to be used, it's hard to suggest any better approach. Why don't you do the calculations where you need them to be done?

davidabian · 2018-07-02T17:03:42Z

Thanks for your comments, @thiemowmde!

As I don't know where exactly this code is supposed to be used, it's hard to suggest any better approach. Why don't you do the calculations where you need them to be done?

Because, to calculate the lower and the higher bounds given a Unix timestamp and a certain precision, you would need to go back to the ISO 8601 format. The constraints use Unix timestamps (currently, getTimestamp()); however, these calculations can't be done with Unix timestamps since, with them, you couldn't know how many seconds you would have to add or substract. After the proposed changes on the TimeValueCalculator, I would replace the use of getTimestamp() with getLowerTimestamp() and getHigherTimestamp() (see RangeCheckerHelper.php). Does this make sense? I think we could even deprecate getTimestamp(), but that's a different story.

I'll check and fix the rest of the issues that you write. Thanks again!

davidabian · 2018-07-02T20:29:23Z

Sorry for so many commits. I can't use the function cal_days_in_month() on HHVM. Is that a problem?

thiemowmde

Please note I'm no longer responsible for this codebase. However, I will happily provide input, as long as it is helpful.

I also realized the TimeValueCalculator class is no longer used for the project it was originally written for, but exclusively used in WikibaseQualityConstraints. This means the team responsible for WikibaseQualityConstraints should feel free to do any change they like to this library. :-)

thiemowmde · 2018-07-03T14:55:12Z

src/DataValues/TimeValueCalculator.php

+				1,
+				-$this->charsAffectedByPrecision( TimeValue::PRECISION_YEAR ) - 1
+			);
+			$daysInMonth = \cal_days_in_month( CAL_GREGORIAN, $month, $year );


cal_days_in_month will fail and return false for many years the Wikibase datatype allows, but PHP does not understand (e.g. the year -100000). I suggest to rely on the same logic as in getSecondsSinceUnixEpoch: $dummyYear = $this->isLeapYear( $year ) ? 1972 : 1970.

Also please add test cases for extreme positive and negative years.

Okay, I'll do it. Thanks!

thiemowmde · 2018-07-03T15:00:00Z

src/DataValues/TimeValueCalculator.php

+	 * Highest positive timestamp strictly earlier than the lowest positive timestamp with
+	 * a length of $MAX_LENGTH_TIMESTAMP + 1.
+	 */
+	private $HIGHEST_TIMESTAMP = '+9999999999999999-12-31T23:59:59Z';


I still feel this needs to be 61 seconds, even if I'm not sure if this is relevant for anything, because wikidata.org currently does not allow precisions smaller than a day anyway.

I think this works as expected using 59 seconds. Next minute is one second from this moment according to Unix timestamps.

thiemowmde · 2018-07-03T15:00:32Z

src/DataValues/TimeValueCalculator.php

+		// WARNING: Day 31 will be applied to all months
+		$timestampCeiling = substr( $timestamp, 0, -$numCharsToModify ) .
+			substr( $this->HIGHEST_TIMESTAMP, -$numCharsToModify );
+		if ( $precision === TimeValue::PRECISION_MONTH ) {


Please add test cases for this month-specific code path.

There's a test for this in testGetLowerTimestamp() and another in testGetHigherTimestamp(). But yes, it would be good to have more, I'll add them.

thiemowmde · 2018-07-03T15:13:09Z

src/DataValues/TimeValueCalculator.php

+	 *
+	 * @return int
+	 */
+	private function charsAffectedByPrecision( $precision ) {


From what I understand this method tells me how many characters can be cut off from the end of the timestamp string. Lets say the given timestamp is +2017-12-31T23:59:59Z:

To get a timestamp with 1 minute precision, you must cut 3 or 4 characters from the end.

To get a timestamp with 1 year precision, you must cut 15 or 16 characters, which results in +2017- or +2017.

To get a timestamp with 10 years precision, you must cut 17 characters, which results in +201. No option here.

I found the numbers confusing first, but can confirm them with this analysis. :-)

Maybe add some explanation to the documentation header of this function?

This is the number of characters from the timestamp string that we should replace with the ones in $TIMESTAMP_ZERO or $HIGHEST_TIMESTAMP. In other words, the lowest number of irrelevant characters in the timestamp string (being irrelevant for exceeding the precision).

I'll add this to the documentation header. Congrats for the analysis, but you could have asked first! ;-)

thiemowmde · 2018-07-03T15:19:02Z

tests/DataValues/TimeValueCalculatorTest.php

+			TimeValue::CALENDAR_GREGORIAN
+		);
+		$unixHigherTimestampAfter2 = $timeValueCalculator->getHigherTimestamp( $timeValueAfter2 );
+		$this->assertLessThan( $unixHigherTimestampAfter2, $unixHigherTimestampAfter1 );


As far as I can see the vast majority of the tests relies on "greater/less than" calculations, but never does strict comparisons. I think this is a problem and hides a lot of possible issues. For example, a timestamp with a precision of a million years might as well give you a bunch of Unix timestamps that are all only a few seconds apart. No assertion here can ever catch this. :-(

lucaswerkmeister

Unfortunately, I don’t have merge rights in this repository, so someone else will still have to take a look at this :/

lucaswerkmeister · 2018-07-10T12:43:30Z

src/DataValues/TimeValueCalculator.php

+	 * Highest positive timestamp strictly earlier than the lowest positive timestamp with
+	 * a length of $MAX_LENGTH_TIMESTAMP + 1.
+	 */
+	private $HIGHEST_TIMESTAMP = '+9999999999999999-12-31T23:59:59Z';


lucaswerkmeister · 2018-07-10T12:50:07Z

tests/DataValues/TimeValueCalculatorTest.php

+	 * @param string $timestamp
+	 * @param int $precision
+	 */
+	private function auxTestGetHigherTimestamp( $timestamp, $precision ) {


Is this function used anywhere? I can’t find any uses of it in this pull request using in-browser search.

No, sorry, I should have removed it.

lucaswerkmeister · 2018-07-10T12:57:16Z

tests/DataValues/TimeValueCalculatorTest.php

+			'245-04-30T00:00:00Z',
+			'1054-02-11T14:00:02Z',
+			'2012-02-29T23:59:59Z',
+			#'2012-03-01T00:00:00Z',


Why is this one commented out?

Tests fail when checking two consecutive BCE dates with the same year. This must be fixed, but I can't easily deal with BCE Unix timestamps, they weren't designed for that. I'm currently not sure what's wrong, the tests or the main functions. Or both. D:

lucaswerkmeister · 2018-07-10T13:03:44Z

tests/DataValues/TimeValueCalculatorTest.php

+	 *   - higher before values for the same timestamp always correspond to lower or equal
+	 *       getLowerTimestamp() values than lower before values,
+	 *   - a few TimeValue values are equal to their expected getLowerTimestamp() ones.
+	 */


I would prefer if these tests were split up into a lot more functions. For example:

One test for checking specific TimeValues and their expected timestamps, with a @dataProvider (note that MediaWiki prefers names like provideTimeValuesWithLowerTimestamps) for the constants that are used below.

One test for comparing the direct timestamp with the lower timestamp, with a data provider for timestamps.

One test for comparing timestamps at different precisions, with the same data provider.

One test for comparing timestamps at different before values, with the same data provider.

One test for checking the ordering of timestamps, with a data provider that returns pairs of timestamps.

Note that the data providers can call each other; for example, given a provideTimestampsWithoutSign (same as your timestampWithoutSignProvider), you could have:

public function provideTimestamps() { // any order foreach ( [ '+', '-' ] as $sign ) { foreach ( $this->provideTimestampsWithoutSign() as $timestamp ) { yield [ $sign . $timestamp ]; } } } public function provideTimestampsAndPrecisions() { foreach ( $this->provideTimestamps() as $timestamp ) { foreach ( $this->providePrecisions() as $precision ) { yield [ $timestamp, $precision ]; } } } public function provideTimestampPairs() { foreach ( [ '+', '-' ] as $sign ) { $oldTimestamp = null; foreach ( $this->provideTimestampsWithoutSign() as $timestamp ) { if ( $oldTimestamp !== null ) { yield $sign === '+' ? [ $oldTimestamp, $timestamp ] : [ $timestamp, $oldTimestamp ]; } $oldTimestamp = $timestamp; } } }

Same thing goes for the getHigherTimestamp tests (they could probably reuse the same data providers).

lucaswerkmeister

Code looks good to me. I’ve also checked the values in provideTimeValuesAndLowerAndHigherTimestamps against GNU date, and the results look correct. If the tests pass, I’m reasonably confident the code is correct :)

Still not happy with the test setup though :P

lucaswerkmeister · 2018-07-13T07:20:53Z

tests/DataValues/TimeValueCalculatorTest.php

+	 * @return \Generator of arrays of:
+	 * - a TimeValue,
+	 * - its lower Unix timestamp, and
+	 * - its Unix higher timestamp.


I feel like this should be consistent, either “lower/higher Unix timestamp” or “Unix lower/higher” timestamp, but not both :)

Oh, right. xD

lucaswerkmeister · 2018-07-13T07:21:27Z

tests/DataValues/TimeValueCalculatorTest.php

+	public function provideTimeValuesAndLowerAndHigherTimestamps() {
+		$timeValuesAndLowerAndHigherTimestamps = [
+			[
+				new TimeValue( // BCE calculations can differ one year from external ones


I’m not sure what this comment means…

Depending on whether you consider year 0 or not, the same BCE timestamp could correspond to a year later. I'm assuming the behavior which is already used in Wikidata and implemented in getTimestamp() is the expected, but just for the record.

lucaswerkmeister · 2018-07-13T07:25:32Z

tests/DataValues/TimeValueCalculatorTest.php

+				5999999999999999
+			],
+		];
+		yield $timeValuesAndLowerAndHigherTimestamps;


Instead of yielding the whole array and then iterating over it in the test functions, why not return the array (or yield the individual elements)?

lucaswerkmeister · 2018-07-13T07:27:12Z

tests/DataValues/TimeValueCalculatorTest.php

+	 * @dataProvider provideTimeValuesAndLowerAndHigherTimestamps
+	 */
+	public function testSpecificLowerTimestamps() {
+		$timeValuesAndLowerTimestamps = func_get_args();


func_get_args() shouldn’t be necessary. With the change in the provider suggested above, the function should take three arguments: $timeValue, $unixLowerTimestamp and $unixHigherTimestamp. (The third would be ignored, unless you merge testSpecificHigherTimestamps() into this test. Either are fine by me.)

lucaswerkmeister · 2018-07-13T07:27:27Z

tests/DataValues/TimeValueCalculatorTest.php

+	 * @dataProvider provideTimeValuesAndLowerAndHigherTimestamps
+	 */
+	public function testSpecificHigherTimestamps() {
+		$timeValuesAndHigherTimestamps = func_get_args();


Same comment as in testSpecificLowerTimestamps() here.

lucaswerkmeister · 2018-07-13T07:28:35Z

tests/DataValues/TimeValueCalculatorTest.php

+						$precision,
+						TimeValue::CALENDAR_GREGORIAN
+					);
+					yield [ [ $oldTimeValue, $timeValue ] ];


This should yield [ $oldTimeValue, $timeValue ] directly, I think, without wrapping it in another array.

lucaswerkmeister · 2018-07-13T07:28:42Z

tests/DataValues/TimeValueCalculatorTest.php

+	public function provideTimestampsAndPrecisions() {
+		foreach ( $this->provideTimestamps() as $timestamp ) {
+			foreach ( $this->provideAllPrecisions() as $precision ) {
+				yield [ [ $timestamp, $precision ] ];


Same here…

lucaswerkmeister · 2018-07-13T07:28:46Z

tests/DataValues/TimeValueCalculatorTest.php

+				if ( $oldTimestamp !== null ) {
+					yield $sign === '+' ?
+						[ [ $oldTimestamp, $timestamp ] ] :
+						[ [ $timestamp, $oldTimestamp ] ];


…and here.

lucaswerkmeister · 2018-07-13T07:30:10Z

tests/DataValues/TimeValueCalculatorTest.php

+	public function testOrderingLowerTimestamps() {
+		$calculator = new TimeValueCalculator();
+		$timestampPairs = func_get_args();
+		foreach ( $timestampPairs as $timestampPair ) {


Similarly, func_get_args() and the loop should not be necessary. The generator should yield a single array of function arguments, and the test function should accept those directly. The same comment applies to the other test functions below as well.

lucaswerkmeister · 2018-07-13T07:48:01Z

I’ve updated the data providers, but I can’t push to this pull request, so I’ve uploaded the patch here: https://gist.github.com/lucaswerkmeister/14a0b55d400345ac3374ee6c2fe14a5d

https://gist.github.com/lucaswerkmeister/14a0b55d400345ac3374ee6c2fe14a5d

davidabian · 2018-07-13T15:14:57Z

Thanks a lot for your time! Maybe @mariushoch can merge?

addshore · 2021-03-18T15:49:01Z

Closing very old PRs

Add getLowerTimestamp() and getHigherTimestamp()

476bf90

Add functions getLowerTimestamp() and getHigherTimestamp(). They'll be used in WikibaseQualityConstraints. PS: I've never programmed in PHP nor I'm familiar with this repository, so please feel free to tell me what I may be doing wrong.

David Abián added 2 commits June 25, 2018 18:09

Update TimeValueCalculatorTest.php

716f066

Update TimeValueCalculatorTest.php

94e77c4

David Abián added 3 commits June 25, 2018 18:24

Missing $this->

4666d27

WMDE code style

d2c4788

WMDE code style

8742851

David Abián added 7 commits July 2, 2018 21:42

Update TimeValueCalculatorTest.php

3a24292

Fix issues noted by @thiemowmde

eef3d79

Old format to make review easier

025d3fc

Old format to make review easier

8d80904

Update TimeValueCalculatorTest.php

f7a6860

Update TimeValueCalculator.php

06fce4f

Try to fix cal_days_in_month on HHVM

c351875

thiemowmde reviewed Jul 3, 2018

View reviewed changes

David Abián added 4 commits July 9, 2018 18:57

Update TimeValueCalculatorTest.php

ae5bb11

Update TimeValueCalculator.php

d18bb9b

Update TimeValueCalculatorTest.php

7425a75

Update TimeValueCalculator.php

6220454

lucaswerkmeister requested changes Jul 10, 2018

View reviewed changes

David Abián added 3 commits July 13, 2018 00:47

Separe tests and using @dataProviders

1c03a7a

Shorter lines

23682dc

Remove comment

2c397e6

lucaswerkmeister requested changes Jul 13, 2018

View reviewed changes

Fix tests, suggestions by @lucaswerkmeister

7271522

https://gist.github.com/lucaswerkmeister/14a0b55d400345ac3374ee6c2fe14a5d

addshore closed this Mar 18, 2021

Add getLowerTimestamp() and getHigherTimestamp() #140

Add getLowerTimestamp() and getHigherTimestamp() #140

Conversation

davidabian commented Jun 25, 2018 • edited Loading

lucaswerkmeister commented Jun 25, 2018

JeroenDeDauw commented Jun 25, 2018 • edited Loading

davidabian commented Jun 25, 2018

davidabian commented Jul 2, 2018 • edited Loading

thiemowmde commented Jul 2, 2018

davidabian commented Jul 2, 2018

davidabian commented Jul 2, 2018

thiemowmde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswerkmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidabian Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswerkmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswerkmeister commented Jul 13, 2018

davidabian commented Jul 13, 2018

addshore commented Mar 18, 2021

davidabian commented Jun 25, 2018 •

edited

Loading

JeroenDeDauw commented Jun 25, 2018 •

edited

Loading

davidabian commented Jul 2, 2018 •

edited

Loading

davidabian Jul 10, 2018 •

edited

Loading