-
Notifications
You must be signed in to change notification settings - Fork 448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storeapi: move from details (v1) to info (v2) #2550
Conversation
Instead of accessing each element from the returned calls as dictionary elements, this implementation offers classes with attributes and some methods to interact with the API results. Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sergiusens
params = dict() | ||
params[ | ||
"fields" | ||
] = "channel-map, snap,snap-id,name,publisher,confinement,revision" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a stray space before snap
, but this field doesn't not exist and it's not needed anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, valid fields are not documented though, are they? they also seem to be flattened, correct?
:return Details for the snap. | ||
:rtype: dict | ||
:return information for the snap. | ||
:rtype: SnapInfo | ||
""" | ||
headers = self.get_default_headers() | ||
headers.update( | ||
{ | ||
"Accept": "application/hal+json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v2 responses are technically only application/json
, we can drop the hal+
part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems good on the Python side but I don't know details about the store API changes, so I'll leave the final approval for @cprov.
Codecov Report
@@ Coverage Diff @@
## master #2550 +/- ##
==========================================
- Coverage 89.02% 88.86% -0.17%
==========================================
Files 201 202 +1
Lines 13611 13772 +161
Branches 2067 2079 +12
==========================================
+ Hits 12117 12238 +121
- Misses 1056 1090 +34
- Partials 438 444 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All points raised by @cprov addressed, approving!
Instead of accessing each element from the returned calls as dictionary
elements, this implementation offers classes with attributes and some
methods to interact with the API results.
Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com
./runtests.sh static
?./runtests.sh tests/unit
?This implementation strays away from the existing implementation pattern of what lives in the storeapi package but I personally think, even if more work initially, will provide easier ways to interact with the results when the Snap Store API is not fresh in your head.