Page MenuHomeSoftware Heritage
Differential D5899

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

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

Details

Reviewers
zack
olasd
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T3393: add swhid() method to from_disk classes
Commits
rDMODe4566a6605ff: from_disk: get swhid from Content/Directory objects
Summary

Related to T3393

Diff Detail

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

Event Timeline

DanSeraf created this revision.Jun 18 2021, 4:48 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 18 2021, 4:48 PM
swh-public-ci added a comment.Jun 18 2021, 4:50 PM

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.

Harbormaster completed remote builds in B22134: Diff 21149.Jun 18 2021, 4:50 PM
DanSeraf requested review of this revision.Jun 18 2021, 4:50 PM
zack added a subscriber: zack.Jun 18 2021, 5:10 PM

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"

498–499

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
zack added subscribers: vlorentz, olasd.Jun 18 2021, 5:13 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!

DanSeraf updated this revision to Diff 21151.Jun 18 2021, 5:22 PM

requested changes

DanSeraf added a comment.Jun 18 2021, 5:23 PM
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)

swh-public-ci added a comment.Jun 18 2021, 5:24 PM

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.

Harbormaster completed remote builds in B22136: Diff 21151.Jun 18 2021, 5:24 PM
zack added a comment.EditedJun 18 2021, 6:33 PM
In D5899#150882, @DanSeraf wrote:

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

DanSeraf updated this revision to Diff 21157.Jun 21 2021, 4:00 PM

unit test

swh-public-ci added a comment.Jun 21 2021, 4:02 PM

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.

Harbormaster completed remote builds in B22144: Diff 21157.Jun 21 2021, 4:03 PM
anlambert added a subscriber: anlambert.Jun 21 2021, 4:34 PM
anlambert added inline comments.
swh/model/from_disk.py
216

Return (capitalize first word for consistency with other docstrings)

498

same here

zack accepted this revision.Jun 21 2021, 4:53 PM
This revision is now accepted and ready to land.Jun 21 2021, 4:53 PM
vlorentz added a comment.Jun 21 2021, 4:56 PM

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

vlorentz added a comment.Jun 21 2021, 5:02 PM

And what about these simplifications?

swh/model/from_disk.py
218–220
500–502
DanSeraf updated this revision to Diff 21168.Jun 21 2021, 5:06 PM

requested changes

swh-public-ci added a comment.Jun 21 2021, 5:08 PM

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.

Harbormaster completed remote builds in B22155: Diff 21168.Jun 21 2021, 5:08 PM
olasd accepted this revision.Jun 21 2021, 5:09 PM

Nice, thanks.

vlorentz accepted this revision.Jun 21 2021, 5:13 PM
Closed by commit rDMODe4566a6605ff: from_disk: get swhid from Content/Directory objects (authored by DanSeraf). · Explain WhyJun 21 2021, 5:16 PM
This revision was automatically updated to reflect the committed changes.
DanSeraf added a commit: rDMODe4566a6605ff: from_disk: get swhid from Content/Directory objects.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
model/
28 lines86%
Loading...
21 lines96%
Loading...
tests/
16 lines97%
Loading...

Diff 21168

View Options

swh/model/cli.py

Loading...
View Options

swh/model/from_disk.py

Loading...
View Options

swh/model/tests/test_from_disk.py

Loading...
About · Developer information · Legal