Page MenuHomeSoftware Heritage

Add a TimestampWithTimezone.to_datetime() method
ClosedPublic

Authored by douardda on Jun 14 2021, 1:49 PM.

Diff Detail

Event Timeline

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.

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

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

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.

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.

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

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.