feat: [sc-47448] implement bookmark methods in Transfer v2 client#1379
Conversation
pjhinton-globus
requested review from
MaxTueckeGlobus,
aaschaer,
ada-globus,
derek-globus,
kurtmckee,
m1yag1 and
sirosen
as code owners
Added the following methods to the experimental TransferClientV2: * create_bookmark() * update_bookmark() * get_bookmark() * delete_bookmark() * list_bookmarks() This provides access to the new Transfer v2 API operations for boomkark management, which includes support for high assurance collections. Functional unit tests are provided for all methods.
pjhinton-globus
force-pushed
the
pjhinton/sc-47448_transfer_v2_bookmark_sdk_support
branch
from
21271f9 to
cf0821c
Compare
src/globus_sdk/experimental/transfer_v2/data/bookmark_documents.py
Outdated
Show resolved
Hide resolved
This commit implements the following requested changes from @kurtmckee in his pull request review. use cross referencing modifier for method name for brevity globus#1379 (comment) fix docstring copy-and-paste typos and verify no other instances elsewhere globus#1379 (comment) look up class name rather than hard-coding in debug log statements globus#1379 (comment) add a missing f-prefix on log string with interpolation globus#1379 (comment)
src/globus_sdk/experimental/transfer_v2/data/bookmark_documents.py
Outdated
Show resolved
Hide resolved
Implements the following requested change from @kurtmckee. globus#1379 (comment)
sirosen
left a comment
sirosen
left a comment
There was a problem hiding this comment.
Some notes worth sharing, but nothing negative and therefore nothing to block approval!
I see there were some changes to logging style in other methods. I'm 👍 on that change itself, but I think that needs to be codified as a standard for us (even if only by policy and code uniformity). I would not have asked for those changes in review here, as our other classes don't follow the self.__class__.__name__ pattern.
Without further action, things will drift. Now that we have started down this road, I think we have to see it through. I'll try to prep a PR when I have a less busy day. I'm curious to see how we can lint for this (static-analysis + runtime inspection?).
@pjhinton-globus, this PR started a couple of threads of conversation among the SDK maintainers, but we don't have a clear-cut plan yet for changes. I think it's important to share back what we're thinking about.
These were the things which were noted as interesting:
- It seems that the JSON:API documents benefit greatly from specialized payload types. Do we want to create a new type for each API with a body? (Across all services, this would be a large number of types.) We're not sure how to handle this.
additional_fieldsis a name that originated from adding data to a subdocument (see:TransferData.add_item); not sure it's the right name for a top-level JSON:API payload. Maybeadditional_attributes?- Should we even have
additional_fields, or an equivalent thereof, for a payload type? You could alwaysdoc = BookmarkCreateDocument(...); doc["data"]["attributes"]["x"] = 1.
Regarding that last item, I think that if we create a base JsonApiPayload(GlobusPayload), we could provide properties to directly access attributes and other important keys. So this would become possible:
doc = BookmarkCreateDocument(...)
doc.attributes["x"] = 1
3 participants
Shortcut Story: https://app.shortcut.com/globus/story/47448/
Added the following methods to the experimental TransferClientV2:
This provides access to the new Transfer v2 API operations for boomkark management, which includes support for high assurance collections.
Functional unit tests are provided for all methods. Change log entry is present.