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 21991 Build 34211: Phabricator diff pipeline on jenkins Jenkins console · Jenkins Build 34210: arc lint + arc unit
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() methodSee 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?
| 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? | |
| swh/model/model.py | ||
|---|---|---|
| 259 | I guess we add a return type annotation | |
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() methodSee 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? | |
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() methodSee 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.