Page MenuHomeSoftware Heritage

Unify object_type some more within the merkle and from_disk modules
ClosedPublic

Authored by ardumont on Jul 2 2020, 11:09 AM.

Diff Detail

Repository
rDMOD Data model
Branch
unify-object-type
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13250
Build 20234: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 20233: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D3390 (id=12030)

Rebasing onto 40a40f508b...

Current branch diff-target is up to date.
Changes applied before test
commit 262d84043df0fb0c91082eb94cc91f4d2a8b9902
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jul 2 10:58:43 2020 +0200

    Unify object_type some more within the merkle and from_disk modules

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

ardumont edited the summary of this revision. (Show Details)
anlambert added a subscriber: anlambert.

LGTM, maybe you could set object_type type as Final like in other model classes (see inline comment).

swh/model/merkle.py
114

Maybe you should type this as Final like in swh.model.model and set some generic object_type value for base merkle model classes ("merkle_node", "merkle_leaf" ?)

Also, maybe the TODO could be removed ?

This revision is now accepted and ready to land.Jul 2 2020, 2:35 PM
swh/model/merkle.py
114

I'll try, I did not in the first place as my understanding of the Final would not work here (base class, etc...).

I left it because I did not really understand it but why not ;)

kinda hope that solves [1]

lazy bones!

[1] https://sentry.softwareheritage.org/share/issue/594af974da3c482b8419cda22a21becc

In the end, that may not be the issue (and it's for the best otherwise, it would have only hidden the bug)

It'd be more like old loaders not properly stopped [2] still running with old code...
I believe the diff is still worth unification though, less to think about for later

[2] P707

swh/model/merkle.py
114

I'll try, I did not in the first place as my understanding of the Final would not work here (base class, etc...).

Oh right, this declare the attribute read only. Maybe remove it from the abstract base class then ?

swh/model/merkle.py
114

I left it because I did not really understand it but why not ;)

it being the todo ;)

Oh right, this declare the attribute read only. Maybe remove it from the abstract base class then ?

i'll try, i'm not so sure mypy will be happy around the lines below then though.

Build is green

Patch application report for D3390 (id=12039)

Rebasing onto 40a40f508b...

Current branch diff-target is up to date.
Changes applied before test
commit 363b1659a6f5fbc32472f0774352d58d068f4282
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jul 2 10:58:43 2020 +0200

    Unify object_type some more within the merkle and from_disk modules

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