Page MenuHomeSoftware Heritage

TimestampWithTimezone: Make 'offset' and 'negative_utc' optional
ClosedPublic

Authored by vlorentz on Thu, Jan 13, 12:14 PM.

Details

Summary

'offset' becomes a property instead of an attribute and constructor argument.

This also removes both from the output of .to_dict().

This is step 2 of T3752

Diff Detail

Repository
rDMOD Data model
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build has FAILED

Patch application report for D6936 (id=25125)

Rebasing onto dbf185c3b3...

Current branch diff-target is up to date.
Changes applied before test
commit 617d79017a01be824e1e453f175a773c53b2f060
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jan 12 15:12:29 2022 +0100

    TimestampWithTimezone: Make 'offset_bytes' required and remove 'negative_utc'.
    
    'offset' becomes a property instead of an attribute and constructor argument.
    
    This also removes both from the output of `.to_dict()`.
    
    This is step 6 of https://forge.softwareheritage.org/T3752
    
    This will break packages that still use the constructor directly,
    ie. swh-storage and swh-loader-git (and tests of swh-loader-core
    and swh-loader-svn)

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

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jan 13, 12:16 PM
Harbormaster failed remote builds in B25995: Diff 25125!

Build is green

Patch application report for D6936 (id=25126)

Rebasing onto dbf185c3b3...

Current branch diff-target is up to date.
Changes applied before test
commit be894c66c88b45f621529df011e1dc5f1de2bd8e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jan 12 15:12:29 2022 +0100

    TimestampWithTimezone: Make 'offset_bytes' required and remove 'negative_utc'.
    
    'offset' becomes a property instead of an attribute and constructor argument.
    
    This also removes both from the output of `.to_dict()`.
    
    This is step 6 of https://forge.softwareheritage.org/T3752
    
    This will break packages that still use the constructor directly,
    ie. swh-storage and swh-loader-git (and tests of swh-loader-core
    and swh-loader-svn)

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

Couldn't we make the old constructor arguments optional first, to avoid breaking all dependencies at once?

We could, but it means another round of going through all four packages and changing every constructor call, and it's a lot of busy work

We could, but it means another round of going through all four packages and changing every constructor call, and it's a lot of busy work

Is it? What I want is for *this* module to support *both* ways of instantiating the object, so we can avoid having to deploy everything at once.

Oh, so you want to make all three parameters optional, but error if we don't have one of the two sets? Hmm... I'll look into it

Looks like we need __attrs_init__ (added by attrs 21.1.0) in order to support this

EDIT: I figured a solution. It's not pretty, but it will work as a temporary workaround.

  • Restore 'offset' and 'negative_utc' arguments and make them optional

Build is green

Patch application report for D6936 (id=25175)

Rebasing onto dbf185c3b3...

Current branch diff-target is up to date.
Changes applied before test
commit 1504ec4b7f7d60c6a162efd66d36f64b2ccbefd2
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Jan 14 13:12:34 2022 +0100

    Restore 'offset' and 'negative_utc' arguments and make them optional
    
    This allows keeping compatibility with existing users of the TimestampWithTimezone constructor

commit be894c66c88b45f621529df011e1dc5f1de2bd8e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jan 12 15:12:29 2022 +0100

    TimestampWithTimezone: Make 'offset_bytes' required and remove 'negative_utc'.
    
    'offset' becomes a property instead of an attribute and constructor argument.
    
    This also removes both from the output of `.to_dict()`.
    
    This is step 6 of https://forge.softwareheritage.org/T3752
    
    This will break packages that still use the constructor directly,
    ie. swh-storage and swh-loader-git (and tests of swh-loader-core
    and swh-loader-svn)

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

Thanks for this change!

We alread have an attrs backport on our systems, so I've prepared a (trivial) backport of attrs 21.2.0; if the version with __attrs_init__ is cleaner, we could go for that.

replace hack with dependency on attrs 21.1.1

Build is green

Patch application report for D6936 (id=25179)

Rebasing onto dbf185c3b3...

Current branch diff-target is up to date.
Changes applied before test
commit b16a587bff89256be767aeb4e7e39bd10b7712ba
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Jan 14 13:12:34 2022 +0100

    Restore 'offset' and 'negative_utc' arguments and make them optional
    
    This allows keeping compatibility with existing users of the TimestampWithTimezone constructor

commit be894c66c88b45f621529df011e1dc5f1de2bd8e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jan 12 15:12:29 2022 +0100

    TimestampWithTimezone: Make 'offset_bytes' required and remove 'negative_utc'.
    
    'offset' becomes a property instead of an attribute and constructor argument.
    
    This also removes both from the output of `.to_dict()`.
    
    This is step 6 of https://forge.softwareheritage.org/T3752
    
    This will break packages that still use the constructor directly,
    ie. swh-storage and swh-loader-git (and tests of swh-loader-core
    and swh-loader-svn)

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

Thanks a lot!

swh/model/model.py
400–402

I assume that these should be marked optional

This revision is now accepted and ready to land.Fri, Jan 14, 1:46 PM

Build is green

Patch application report for D6936 (id=25180)

Rebasing onto dbf185c3b3...

Current branch diff-target is up to date.
Changes applied before test
commit a0f5436273ef6f5b62a388ae131ed5afa7287d00
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Jan 14 13:12:34 2022 +0100

    Restore 'offset' and 'negative_utc' arguments and make them optional
    
    This allows keeping compatibility with existing users of the TimestampWithTimezone constructor

commit be894c66c88b45f621529df011e1dc5f1de2bd8e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jan 12 15:12:29 2022 +0100

    TimestampWithTimezone: Make 'offset_bytes' required and remove 'negative_utc'.
    
    'offset' becomes a property instead of an attribute and constructor argument.
    
    This also removes both from the output of `.to_dict()`.
    
    This is step 6 of https://forge.softwareheritage.org/T3752
    
    This will break packages that still use the constructor directly,
    ie. swh-storage and swh-loader-git (and tests of swh-loader-core
    and swh-loader-svn)

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

swh/model/model.py
400–402

nope, I don't want callers to pass None.

swh/model/model.py
400–402

Ah, right.

vlorentz retitled this revision from TimestampWithTimezone: Make 'offset_bytes' required and remove 'negative_utc'. to TimestampWithTimezone: Make 'offset' and 'negative_utc' optinoal.Fri, Jan 14, 1:54 PM
vlorentz edited the summary of this revision. (Show Details)
vlorentz retitled this revision from TimestampWithTimezone: Make 'offset' and 'negative_utc' optinoal to TimestampWithTimezone: Make 'offset' and 'negative_utc' optional.Fri, Jan 14, 1:57 PM