Details

Reviewers

vlorentz
ardumont

Group Reviewers

Reviewers

Maniphest Tasks

T2421: Make model objects immutable

Commits

rDMOD3d9f69444aeb: Use Tuple instead of List in model declarations.

Summary

Use Tuple instead of List in model declaration.

Diff Detail

Repository

rDMOD Data model

Branch

tuples

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 12476
Build 18938: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 18937: arc lint + arc unit

Event Timeline

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

Herald added a reviewer: Reviewers. · View Herald TranscriptMay 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

Harbormaster failed remote builds in B12474: Diff 11284!May 25 2020, 1:46 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

Harbormaster failed remote builds in B12475: Diff 11285!May 25 2020, 2:00 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.

Harbormaster completed remote builds in B12476: Diff 11286.May 25 2020, 2:16 PM

What is the motivation for this?

In D3177#77343, @vlorentz wrote:

What is the motivation for this?

Be able to create sets of revisions.

In D3177#77353, @douardda wrote:

In D3177#77343, @vlorentz wrote:

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.

In D3177#77354, @douardda wrote:

In D3177#77353, @douardda wrote:

In D3177#77343, @vlorentz wrote:

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.

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.

Harbormaster completed remote builds in B12478: Diff 11287.May 25 2020, 2:56 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

In D3177#77360, @vlorentz wrote:

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...

In D3177#77371, @douardda wrote:

In D3177#77360, @vlorentz wrote:

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.

douardda mentioned this in T2421: Make model objects immutable.May 26 2020, 4:52 PM

douardda added a task: T2421: Make model objects immutable.May 26 2020, 5:04 PM

douardda mentioned this in T2423: Extract the `extra_headers` away from `Revision.metadata` into a top-level immutable object.May 26 2020, 5:07 PM

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

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.

Harbormaster completed remote builds in B12575: Diff 11377.Jun 2 2020, 2:20 PM

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)

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

Closed by rDMOD3d9f69444aeba903f701486d12978ea887a0a8f8

ardumont mentioned this in D3211: cassandra.converters: Fix revision type.Jun 3 2020, 2:45 PM

ardumont mentioned this in D3217: loader-core: Adapt build_revision implementation according to latest model api.Jun 3 2020, 4:42 PM

ardumont mentioned this in rDLDBASE29bf279ca58a: Adapt build_revision implementation according to latest model api.Jun 4 2020, 11:40 AM

ardumont mentioned this in rDLDHG03c34b9efd19: mercurial.loader: Fix revision parents field to a tuple.Jun 4 2020, 11:43 AM