Page MenuHomeSoftware Heritage

swh-model: get SWHID from Content/Directory objects in from_disk
ClosedPublic

Authored by DanSeraf on Jun 18 2021, 4:48 PM.

Diff Detail

Repository
rDMOD Data model
Branch
from_disk-swhid
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22136
Build 34443: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 34442: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D5899 (id=21149)

Rebasing onto e09446a6f4...

Current branch diff-target is up to date.
Changes applied before test
commit 4a3289f6c1ac9fe1254e45e0f5adf6388b69fb30
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Fri Jun 18 15:45:10 2021 +0100

    from_disk: get swhid from Content/Directory objects

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

LGTM in general, but needs unit tests (in addition to the two nitpicks above about docstrings)

swh/model/from_disk.py
215

you need a docstring here, like "return node identifier as a SWHID"

499–500

better docstring: "return node identifier as a SWHID"

zack requested changes to this revision.Jun 18 2021, 5:10 PM
This revision now requires changes to proceed.Jun 18 2021, 5:10 PM

@olasd @vlorentz: I've noted down only nits and "classics" in the above review. If you want to chime in on the approach (where the methods are, properties, caching, etc.) please do!

In D5899#150867, @zack wrote:

LGTM in general, but needs unit tests (in addition to the two nitpicks above about docstrings)

I'll wait @vlorentz and @olasd before to write the unit test (in the case they want to use a different approach)

Build is green

Patch application report for D5899 (id=21151)

Rebasing onto e09446a6f4...

Current branch diff-target is up to date.
Changes applied before test
commit 949098fc1857306ec16e384d2f9c06c330b2ea29
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Fri Jun 18 15:45:10 2021 +0100

    from_disk: get swhid from Content/Directory objects

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

I'll wait @vlorentz and @olasd before to write the unit test (in the case they want to use a different approach)

I think you can just go ahead, in the sense that the "hard" part here will be to find the test cases (and implement the needed scaffolding) in the test suite.
Even if the approach changes, it will just be a different method/attribute to change, which won't impact the unit tests much (it will "just" impact your implementation of those methods/attributes).

Build is green

Patch application report for D5899 (id=21157)

Rebasing onto e09446a6f4...

Current branch diff-target is up to date.
Changes applied before test
commit 1a63184095e8ce126821a4c39cef0e157a4b51dc
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Fri Jun 18 15:45:10 2021 +0100

    from_disk: get swhid from Content/Directory objects

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

anlambert added inline comments.
swh/model/from_disk.py
216

Return (capitalize first word for consistency with other docstrings)

499

same here

This revision is now accepted and ready to land.Jun 21 2021, 4:53 PM

Could you make it a method instead of a property, for consistency with swh.model.model?

And what about these simplifications?

swh/model/from_disk.py
218–220
501–503

Build is green

Patch application report for D5899 (id=21168)

Rebasing onto e09446a6f4...

Current branch diff-target is up to date.
Changes applied before test
commit e4566a6605ff7896a1701892faac3f2ec9ea7d10
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Fri Jun 18 15:45:10 2021 +0100

    from_disk: get swhid from Content/Directory objects
    
    Closes T3393

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