Paths

Table of Contentst

Make snapshot_get_branches return a TypedDict containing SnapshotBranch objects.
ClosedPublic
Actions

Authored by vlorentz on Aug 7 2020, 2:04 PM.

Details

Reviewers

ardumont
anlambert

Group Reviewers

Reviewers

Commits

rDSTO4918759fc843: Make snapshot_get_branches return a TypedDict containing SnapshotBranch objects.

Summary

Instead of untyped dictionaries.

This makes snapshot_get longer and duplicates its code across backends;
but snapshot_get should be removed soon.

Related: T645

Diff Detail

Repository

rDSTO Storage manager

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Aug 7 2020, 2:04 PM

Build is green

Patch application report for D3740 (id=13167)

Rebasing onto d9ff3912d5...

Current branch diff-target is up to date.

Changes applied before test

commit 9235cb4e7ba2ffe6d63c0d9278807c9f640d24ee
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Aug 7 14:04:37 2020 +0200

    Make snapshot_get_branches return a TypedDict containing SnapshotBranch objects.
    
    Instead of untyped dictionaries.
    
    This makes snapshot_get longer and duplicates its code across backends;
    but snapshot_get should be removed soon.
    
    Related: T645

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

Harbormaster completed remote builds in B14433: Diff 13167.Aug 7 2020, 2:09 PM

vlorentz mentioned this in D3741: Update for the new type of snapshot_get_branches in swh-storage > 0.13.0.Aug 7 2020, 2:26 PM

anlambert added a subscriber: anlambert.Aug 7 2020, 2:27 PM

anlambert added inline comments.

swh/storage/interface.py
43	I am not a big fan of that naming. For small snapshots, the returned branches set will not be partial for instance. How about `BranchesResultSet` instead ?

anlambert added inline comments.Aug 7 2020, 2:28 PM

swh/storage/interface.py
43	or `BranchesPagedResult` ?

vlorentz added inline comments.Aug 7 2020, 2:33 PM

swh/storage/interface.py
43	I couldn't find a better name. `ResultSet` name-clashes with cassandra (plus, it doesn't make it obvious it's only a partial view), and `PagedResult` with the class with that name.

anlambert added inline comments.Aug 7 2020, 2:40 PM

swh/storage/interface.py
43	How about simply `Branches` then ? `def snapshot_get_branches(..) -> Optional[Branches]` Not a shocker to me.

vlorentz added inline comments.Aug 7 2020, 3:29 PM

swh/storage/interface.py
43	it still doesn't page it clear it's a partial view.

ardumont added inline comments.Aug 7 2020, 3:30 PM

swh/storage/interface.py
43	I proposed in T645 that the method be typed [1] `def snapshot_get_branches(..) -> Optional[PagedResult[SnapshotBranch]]` Optional in case the snapshot is (Sha1Git) does not exist... (or raise to avoid the specific type...) Paged. The PagedResult implies the result can be partial... To have a full snapshot, the client needs to call the `algo.snapshot_get_full` endpoint (<- use the real name which escapes me right now me ;) wdyt? [1] T645#47156

vlorentz added inline comments.Aug 7 2020, 3:31 PM

swh/storage/interface.py
43	PagedResult isn't great, because snapshot_get_branches doesn't use an opaque token for pagination.

ardumont added inline comments.Aug 7 2020, 3:32 PM

swh/storage/interface.py
43	Optional in case the snapshot is (Sha1Git) does not exist... (or raise to avoid the specific type...) Paged. sorry for the typos, one must read... Optional in case the snapshot id (Sha1Git) does not exist... (or raise to avoid the specific case of the inexisting snapshot)
43	PagedResult isn't great, because snapshot_get_branches doesn't use an opaque token for pagination. What's preventing us from doing it that? The snapshot branches order is not guaranteed, isn't it?

ardumont added inline comments.Aug 7 2020, 3:34 PM

swh/storage/interface.py
43	doing that way*... (need some sleep, sorry again...)

vlorentz added inline comments.Aug 7 2020, 3:36 PM

swh/storage/interface.py
43	What's preventing us from doing it that? Nothing, but there is no reason to switch to an opaque token. The snapshot branches order is not guaranteed, isn't it? It is; snapshot branches are sorted by name.

anlambert added inline comments.Aug 7 2020, 3:45 PM

swh/storage/interface.py
43	The fact that the returned branches set might be partial is clearly indicated in the docstring. If you want to keep the `Partial` prefix, at least remove the `Dict` suffix as it is not really needed.

Remove 'Dict' suffix.

Looks good to me.

This revision is now accepted and ready to land.Aug 7 2020, 3:54 PM

Build is green

Patch application report for D3740 (id=13179)

Rebasing onto d9ff3912d5...

Current branch diff-target is up to date.

Changes applied before test

commit 4918759fc84376b16ebf82c60da9d2c17d265821
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Aug 7 14:04:37 2020 +0200

    Make snapshot_get_branches return a TypedDict containing SnapshotBranch objects.
    
    Instead of untyped dictionaries.
    
    This makes snapshot_get longer and duplicates its code across backends;
    but snapshot_get should be removed soon.
    
    Related: T645

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

Harbormaster completed remote builds in B14445: Diff 13179.Aug 7 2020, 3:56 PM

yeah, nice!

Closed by commit rDSTO4918759fc843: Make snapshot_get_branches return a TypedDict containing SnapshotBranch objects. (authored by vlorentz). · Explain WhyAug 7 2020, 6:13 PM

This revision was automatically updated to reflect the committed changes.

vlorentz added a commit: rDSTO4918759fc843: Make snapshot_get_branches return a TypedDict containing SnapshotBranch objects..