Page MenuHomeSoftware Heritage

Use Tuple instead of List in model declarations
ClosedPublic

Authored by douardda on May 25 2020, 1:44 PM.

Details

Summary

Use Tuple instead of List in model declaration.

Diff Detail

Event Timeline

douardda created this revision.May 25 2020, 1:44 PM

Build has FAILED

Patch application report for D3177 (id=11284)

Rebasing onto 29312dff6d...

Current branch diff-target is up to date.
Changes applied before test
commit e88599c843d123e7689c39a072600c48c3050d80
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon May 25 13:17:00 2020 +0200

    Make all model objects hashable
    
    Use Tuple instead of List in model declaration.
    
    This requires currently not yet published attrs_strict PR #38.

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

douardda updated this revision to Diff 11285.May 25 2020, 1:58 PM

Bump dep on attrs_strict >= 0.0.7

now it's on pypi.

Build has FAILED

Patch application report for D3177 (id=11285)

Rebasing onto 29312dff6d...

Current branch diff-target is up to date.
Changes applied before test
commit 50ba1c389f0b64bfacb07de4bb1152bc87bb67d8
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon May 25 13:17:00 2020 +0200

    Make all model objects hashable
    
    Use Tuple instead of List in model declaration.
    
    This requires attrs_strict >= 0.0.7.

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

douardda updated this revision to Diff 11286.May 25 2020, 2:14 PM

Forgot to update hypothesis strategies also.

Build is green

Patch application report for D3177 (id=11286)

Rebasing onto 29312dff6d...

Current branch diff-target is up to date.
Changes applied before test
commit 9cb641e3d52ba468f8a28fde05507752764da6c9
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon May 25 13:17:00 2020 +0200

    Make all model objects hashable
    
    Use Tuple instead of List in model declaration.
    
    This requires attrs_strict >= 0.0.7.

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

What is the motivation for this?

What is the motivation for this?

Be able to create sets of revisions.

What is the motivation for this?

Be able to create sets of revisions.

Also, having frozen objects that are modifiable seems a bit fuzzy to me, to say the least.

What is the motivation for this?

Be able to create sets of revisions.

Also, having frozen objects that are modifiable seems a bit fuzzy to me, to say the least.

But I agree I should have explained the "why", and not (only) the "what", in the commit message.

douardda updated this revision to Diff 11287.May 25 2020, 2:54 PM

improve the commit message

Build is green

Patch application report for D3177 (id=11287)

Rebasing onto 29312dff6d...

Current branch diff-target is up to date.
Changes applied before test
commit ffb0ba3f719517bed7ae43464ee403ec36598970
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon May 25 13:17:00 2020 +0200

    Make all model objects actually frozen and thus hashable
    
    Use Tuple instead of List in model declarations. This is needed so all model
    objects, declared as frozen, are really no editable.
    
    This also allows to create sets of model objects (especially for Revision)
    which can be handy for tests (eg. when testing a replayer scenario, checking
    all objects of a type has been replayed using simple comparisons of sets.)
    
    This requires attrs_strict >= 0.0.7.

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

vlorentz requested changes to this revision.May 25 2020, 3:05 PM

We wouldn't need that if pytest had an equivalent for assertCountEqual :/

And this diff is missing tests to check the objects are indeed hashable

This revision now requires changes to proceed.May 25 2020, 3:05 PM

We wouldn't need that if pytest had an equivalent for assertCountEqual :/

That's only a side effect, even if it's the original reason I started this. Having a mutable "frozen" object is just bad design IMHO.

And this diff is missing tests to check the objects are indeed hashable

Yes, I've added one, which fails due to dicts...

olasd added a subscriber: olasd.EditedMay 26 2020, 10:49 AM

And this diff is missing tests to check the objects are indeed hashable

Yes, I've added one, which fails due to dicts...

We need to move the extra_headers away from Revision.metadata into a top-level, immutable object, as this is a significant field that is used in the revision id.

Once that's done, we can ignore the fact that we still have mutable metadata in these objects, considering that the field is on the way out, superseded by the extrinsic metadata storage which should be happening real soon.

The commit message should at least be amended, since it does not make all model objects immutable, but only fix a couple of model classes.

Bump dep on attrs_strict >= 0.0.7
now it's on pypi.

Now it's on our debian repository as well :D
I have taken the opportunity to also update our wiki page on debian packaging \m/ [1]

[1] https://wiki.softwareheritage.org/wiki/Debian_packaging#Updating_a_dependency_packaging_repository

douardda updated this revision to Diff 11377.Jun 2 2020, 2:19 PM

rebase + rephrase the commit message

Build is green

Patch application report for D3177 (id=11377)

Rebasing onto a95646fc87...

Current branch diff-target is up to date.
Changes applied before test
commit bc2c250b60db9b8562c60cafcb2fdda7ac6cc6b8
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon May 25 13:17:00 2020 +0200

    Use Tuple instead of List in model declarations.
    
    This is a step forward having model objects, declared as frozen, immutable.
    
    This requires attrs_strict >= 0.0.7.

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

douardda retitled this revision from Make all model objects hashable to Use Tuple instead of List in model declarations.Jun 2 2020, 2:37 PM
douardda edited the summary of this revision. (Show Details)
ardumont accepted this revision.Jun 2 2020, 3:49 PM

Looks fine to me (and in accord with the associated task discussion ;)

vlorentz accepted this revision.Jun 2 2020, 5:06 PM
This revision is now accepted and ready to land.Jun 2 2020, 5:06 PM
douardda closed this revision.Jun 4 2020, 11:46 AM

closed by 3d9f69444aeba903f701486d12978ea887a0a8f8