Skip to content
Snippets Groups Projects

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

Closed swh-model: get SWHID from Content/Directory objects in from_disk
generated-differential-D5899-source into generated-differential-D5899-target
4 unresolved threads
Closed Daniele Serafini requested to merge generated-differential-D5899-source into generated-differential-D5899-target
4 unresolved threads

Related to #3393 (closed)

Migrated from D5899 (view on Phabricator)

Merge request reports

Approval is optional

Closed by Phabricator Migration userPhabricator Migration user 3 years ago (Jun 21, 2021 3:16pm UTC)

Merge details

  • The changes were not merged into generated-differential-D7930-target.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
swh/model/from_disk.py
207 212 obj = cls(ret)
208 213 return obj
209 214
215 def swhid(self) -> CoreSWHID:
216 """Return node identifier as a SWHID
  • Stefano Zacchiroli
    Stefano Zacchiroli @zack started a thread on the diff
    • swh/model/from_disk.py
    482 494
    483 495 return self.__entries
    484 496
    497 def swhid(self) -> CoreSWHID:
    498 """Return node identifier as a SWHID
    499 """
  • Stefano Zacchiroli
    Stefano Zacchiroli @zack ·
    Maintainer

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

  • Stefano Zacchiroli
    Stefano Zacchiroli @zack ·
    Maintainer

    Merge request was returned for changes

  • Stefano Zacchiroli
    Stefano Zacchiroli @zack ·
    Maintainer

    @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!

  • Daniele Serafini
    Daniele Serafini @DanSeraf ·
    Author

    requested changes

  • Daniele Serafini
    Daniele Serafini @DanSeraf ·
    Author

    ! In !189 (closed), @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)

  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build is green

    Patch application report for D5899 (id=21151)

    Rebasing onto e09446a6...

    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.

  • Stefano Zacchiroli
    Stefano Zacchiroli @zack ·
    Maintainer

    ! In !189 (closed), @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).

  • Daniele Serafini
    Daniele Serafini @DanSeraf ·
    Author

    unit test

  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build is green

    Patch application report for D5899 (id=21157)

    Rebasing onto e09446a6...

    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.

  • Antoine Lambert
    Antoine Lambert @anlambert started a thread on the diff
    • swh/model/from_disk.py
    207 212 obj = cls(ret)
    208 213 return obj
    209 214
    215 def swhid(self) -> CoreSWHID:
    216 """Return node identifier as a SWHID
    217 """
  • Antoine Lambert
    Antoine Lambert @anlambert started a thread on the diff
    • swh/model/from_disk.py
    482 494
    483 495 return self.__entries
    484 496
    497 def swhid(self) -> CoreSWHID:
    498 """Return node identifier as a SWHID
    499 """
    500 return CoreSWHID(
  • Stefano Zacchiroli
    Stefano Zacchiroli @zack ·
    Maintainer

    Merge request was accepted

  • Stefano Zacchiroli approved this merge request

    approved this merge request

  • vlorentz
    vlorentz @vlorentz ·
    Maintainer

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

  • vlorentz
    vlorentz @vlorentz ·
    Maintainer

    And what about these simplifications?

  • Daniele Serafini
    Daniele Serafini @DanSeraf ·
    Author

    requested changes

  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build is green

    Patch application report for D5899 (id=21168)

    Rebasing onto e09446a6...

    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 #3393

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

  • Nicolas Dandrimont
    Nicolas Dandrimont @olasd ·
    Maintainer

    Nice, thanks.

  • Daniele Serafini
    Daniele Serafini @DanSeraf ·
    Author

    Merge request was merged

  • Daniele Serafini closed

    closed

    • Please register or sign in to reply