-
Notifications
You must be signed in to change notification settings - Fork 98
Add identification headers to SDK communication with servers #754
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
Conversation
Removes the Request class from model module. Paged now is initialized with the first page JSON and a callable to retrieve subsequent pages JSON. Response now only stores what is necessary from the http response and does not store the reqest. Adds StreamingResponse to support StreamingBody with additional properties. Removes Response last_modified property as it is unused.
remove Response class and have Session make requests Paged is initiated with a response instead of a request Remove Stream class and have Session.stream be a context manager yielding a streaming response move orders client to new request/response model
… to use new sessions.request format
| response = await self._do_request(request) | ||
| response = await self._session.request(method='PUT', | ||
| url=url, | ||
| json=request) |
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.
@jreiberkyle @kevinlacaille tangential comment here: the code might be more readable if we didn't use "request" as both a verb and a noun. Maybe self._session.request could be changed to self._session.send someday?
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.
Ohh yeah I fully agree here! Right now Session has two methods that communicate with the servers: request() and stream(). Both of these suffer from verb/noun ambiguity. This was some of the satisfaction in removing the Request and Stream classes. At least we no longer have noun representation in the code base.
| url=url, | ||
| params=params) | ||
| async for sub in _SubscriptionsPager(response, | ||
| self._session.request, |
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.
Great.
| }, 'items': [1, 2] | ||
| }) | ||
| async def get_response(url, method): | ||
| return resp |
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.
I like that the pager change makes testing more clear.
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.
yeah, the change was actually partially informed by the test formation. That was nice. Revisiting one's old code and making it smoother with the benefit of hindsight is very satisfying.
sgillies
left a comment
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.
@jreiberkyle excellent 🚀
Related Issue(s):
Closes #739
Proposed Changes:
For inclusion in changelog:
Not intended for changelog:
Diff of User Interface
Old behavior:
From the code and output below, this is the relevant line:
Note that the headers 'x-planet-app' and 'user-agent' are not included.
New behavior:
From the code and output below, this is the relevant line:
Note that the headers 'x-planet-app' and 'user-agent' are now included.
PR Checklist:
(Optional) @mentions for Notifications:
@cholmes @sgillies