Details

Reviewers

aeviso
olasd

Group Reviewers

Reviewers

Commits

rDMOD8ec0cf685bfb: Add a TimestampWithTimezone.to_datetime() method

Diff Detail

Repository

rDMOD Data model

Branch

master

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 21987
Build 34204: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 34203: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	3 ms	Jenkins > .tox.py3-full.lib.python3.7.site-packages.swh.model.tests.test_identifiers::test_normalize_timestamp_datetime[0-tz0--1439-date0-1582810759] date = datetime.datetime(2020, 2, 26, 14, 40, 19, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=60))) seconds = 1582810759 tz = datetime.timezone(datetime.timedelta(days=-1, seconds=60)), offset = -1439
	2 ms	Jenkins > .tox.py3-full.lib.python3.7.site-packages.swh.model.tests.test_identifiers::test_normalize_timestamp_datetime[0-tz0--1439-date1-4765129199] date = datetime.datetime(2120, 12, 31, 0, 0, 59, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=60))) seconds = 4765129199 tz = datetime.timezone(datetime.timedelta(days=-1, seconds=60)), offset = -1439
	2 ms	Jenkins > .tox.py3-full.lib.python3.7.site-packages.swh.model.tests.test_identifiers::test_normalize_timestamp_datetime[0-tz0--1439-date2--11348929581] date = datetime.datetime(1610, 5, 13, 15, 44, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=60))) seconds = -11348929581 tz = datetime.timezone(datetime.timedelta(days=-1, seconds=60)), offset = -1439
	2 ms	Jenkins > .tox.py3-full.lib.python3.7.site-packages.swh.model.tests.test_identifiers::test_normalize_timestamp_datetime[0-tz1--60-date0-1582810759] date = datetime.datetime(2020, 2, 27, 13, 39, 19, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=82800))) seconds = 1582810759 tz = datetime.timezone(datetime.timedelta(days=-1, seconds=82800)), offset = -60
	2 ms	Jenkins > .tox.py3-full.lib.python3.7.site-packages.swh.model.tests.test_identifiers::test_normalize_timestamp_datetime[0-tz1--60-date1-4765129199] date = datetime.datetime(2120, 12, 31, 22, 59, 59, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=82800))) seconds = 4765129199 tz = datetime.timezone(datetime.timedelta(days=-1, seconds=82800)), offset = -60
		View Full Test Results (90 Failed · 563 Passed · 2 Skipped)

Event Timeline

douardda created this revision.Jun 14 2021, 1:49 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptJun 14 2021, 1:49 PM

Build is green

Patch application report for D5861 (id=20984)

Rebasing onto ae50e43fe0...

Current branch diff-target is up to date.

Changes applied before test

commit b2e209f881c765a3bdd054351092ed9682499ecd
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Jun 14 13:48:13 2021 +0200

    Add a TimestampWithTimezone.to_datetime() method

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/344/ for more details.

Harbormaster completed remote builds in B21964: Diff 20984.Jun 14 2021, 1:51 PM

douardda requested review of this revision.Jun 14 2021, 1:52 PM

Could you mention in the docstring that to_datetime is lossy (negative UTC) and may crash because of overflows?

olasd added a subscriber: olasd.Jun 14 2021, 1:55 PM

olasd added inline comments.

swh/model/tests/test_model.py
333–341	Could you add a check for the value of the timezone? (i.e. checking that the utcoffset passes through) Could you also add a check for a TimestampWithTimezone before the epoch?

aeviso requested changes to this revision.Jun 14 2021, 2:14 PM

aeviso added a subscriber: aeviso.

aeviso added inline comments.

swh/model/model.py
259	I guess we add a return type annotation

This revision now requires changes to proceed.Jun 14 2021, 2:14 PM

docstring, annotation and better testing

aeviso accepted this revision as: aeviso.Jun 14 2021, 2:26 PM

Build is green

Patch application report for D5861 (id=20990)

Rebasing onto ae50e43fe0...

Current branch diff-target is up to date.

Changes applied before test

commit ca34e90e411abb4aa933ab075e34312cd3412f19
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Jun 14 13:48:13 2021 +0200

    Add a TimestampWithTimezone.to_datetime() method

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/345/ for more details.

Harbormaster completed remote builds in B21970: Diff 20990.Jun 14 2021, 2:27 PM

LGTM with a few comments that you should feel free to apply or not :-)

swh/model/model.py
260
swh/model/tests/test_model.py
341	My main concern was about pre-epoch timestamps with a non-zero amount of microseconds, but I hadn't written that so you couldn't have guessed (it has been an issue/corner case that we've had to fix before). Could you add that to one of the tests?

This revision is now accepted and ready to land.Jun 14 2021, 4:12 PM

add tests for dates < epoch and microseconds > 0...

... and fix the code, because.

Build is green

Patch application report for D5861 (id=20995)

Rebasing onto ae50e43fe0...

Current branch diff-target is up to date.

Changes applied before test

commit 79d2ec502e517de9809892ced817f6b3e67014d5
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Jun 14 13:48:13 2021 +0200

    Add a TimestampWithTimezone.to_datetime() method

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/346/ for more details.

Harbormaster completed remote builds in B21974: Diff 20995.Jun 14 2021, 5:31 PM

douardda added a parent revision: D5868: Fix normalize_timestamp() for datetime < epoch with microsecond>0.Jun 15 2021, 10:47 AM

revert the wrong fix but make it depend on D5868 instead

since this later is the proper fix

Build has FAILED

Patch application report for D5861 (id=21009)

Could not rebase; Attempt merge onto ae50e43fe0...

Updating ae50e43..0940b0b
Fast-forward
 swh/model/identifiers.py            |  4 +++-
 swh/model/model.py                  | 15 +++++++++++++++
 swh/model/tests/test_identifiers.py | 28 ++++++++++++++++++++++++++++
 swh/model/tests/test_model.py       | 13 +++++++++++++
 4 files changed, 59 insertions(+), 1 deletion(-)

Changes applied before test

commit 0940b0bbd46dccd5150b36a2be12f836a357a7ff
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Jun 14 13:48:13 2021 +0200

    Add a TimestampWithTimezone.to_datetime() method

commit 171246dae112447a97e9ad742ad13546e0667da5
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Jun 15 10:39:21 2021 +0200

    Fix normalize_timestamp() for datetime < epoch with microsecond>0
    
    the problem was for datetime<epoch, the timestamp is negative, but since
    it's a float that includes the microseconds, if both are true (< epoch
    and microsecond > 0), then the computed (int) timestamp was off by one.
    
    Add dedicated tests for this.

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/348/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/348/console

Harbormaster failed remote builds in B21987: Diff 21009!Jun 15 2021, 10:51 AM

rebase

Build is green

Patch application report for D5861 (id=21012)

Could not rebase; Attempt merge onto ae50e43fe0...

Updating ae50e43..8ec0cf6
Fast-forward
 swh/model/identifiers.py            |  4 +++-
 swh/model/model.py                  | 15 +++++++++++++++
 swh/model/tests/test_identifiers.py | 29 +++++++++++++++++++++++++++++
 swh/model/tests/test_model.py       | 13 +++++++++++++
 4 files changed, 60 insertions(+), 1 deletion(-)

Changes applied before test

commit 8ec0cf685bfb7ffefca1a7b0a26c7dff142e53f3
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Jun 14 13:48:13 2021 +0200

    Add a TimestampWithTimezone.to_datetime() method

commit 428c170496ef53f6602bc35ab0c9b60fea8996e4
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Jun 15 10:39:21 2021 +0200

    Fix normalize_timestamp() for datetime < epoch with microsecond>0
    
    the problem was for datetime<epoch, the timestamp is negative, but since
    it's a float that includes the microseconds, if both are true (< epoch
    and microsecond > 0), then the computed (int) timestamp was off by one.
    
    Add dedicated tests for this.

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/350/ for more details.

Harbormaster completed remote builds in B21991: Diff 21012.Jun 15 2021, 11:29 AM

Closed by commit rDMOD8ec0cf685bfb: Add a TimestampWithTimezone.to_datetime() method (authored by douardda). · Explain WhyJun 15 2021, 2:46 PM

This revision was automatically updated to reflect the committed changes.

douardda added a commit: rDMOD8ec0cf685bfb: Add a TimestampWithTimezone.to_datetime() method.

Add a TimestampWithTimezone.to_datetime() method
ClosedPublic
Actions

Details

Diff Detail

Unit TestsFailed
View All

Event Timeline

Patch application report for D5861 (id=20984)

Changes applied before test

Patch application report for D5861 (id=20990)

Changes applied before test

Patch application report for D5861 (id=20995)

Changes applied before test

Patch application report for D5861 (id=21009)

Changes applied before test

Patch application report for D5861 (id=21012)

Changes applied before test

Revision Contents
Changeset List

Diff 21009

swh/model/model.py

swh/model/tests/test_model.py

Add a TimestampWithTimezone.to_datetime() methodClosedPublicActions

Details

Diff Detail

Unit TestsFailedView All

Event Timeline

Patch application report for D5861 (id=20984)

Changes applied before test

Patch application report for D5861 (id=20990)

Changes applied before test

Patch application report for D5861 (id=20995)

Changes applied before test

Patch application report for D5861 (id=21009)

Changes applied before test

Patch application report for D5861 (id=21012)

Changes applied before test

Revision ContentsChangeset List

Diff 21009

swh/model/model.py

swh/model/tests/test_model.py

Add a TimestampWithTimezone.to_datetime() method
ClosedPublic
Actions

Unit TestsFailed
View All

Revision Contents
Changeset List