-
Notifications
You must be signed in to change notification settings - Fork 1
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
API endpoint methods should return response objects #12
Comments
@Quantra One of the basic design decisions that we made when designing the library was that we were going to operate using normal python objects (dicts and lists) rather than special API classes with associated behavioral methods. This makes it much easier to interoperate with raw API requests where necessary. That design decision was primarily focused on how items are manipulated - for example, using Your proposed solution seems correct. We would refactor and redefine the API call outputs to return the This can be implemented. As it is a breaking change, it will require perhaps a major version bump. We'll make a test case to illustrate the difference and discuss it internally. |
@yqdv no problem! I have been guilty of the same more than once. With the power of hindisght I think your design decision has merit. For my project I wrote a small API client which returned response objects. Because I'm using Celery I then need to perform some processing of the response object in order for it to be JSON serializable. So in this case having a dict returned from the API would have been beneficial. So perhaps it would be better to stay with a dict but expand it to include more data as this would fit better with your design? This is the dict I save in each task:
And here is the client I wrote: https://gist.github.com/Quantra/5aba08620cb85c0d6fa3896232f1cb46 Whilst my client is somewhat stringly typed it is suitable for my needs and I have no intention to replace it. So on that basis please feel free to close out this issue and issue #11. Briefly my project and use case is a larger management system that has an Opalstack dashboard component where I can manage multiple accounts connected via API keys. If it would be beneficial to you/Opalstack or just of interest to know more please message/email directly =] |
@Quantra Thanks a lot for your comment; it elucidates the motivation for your approach in a practical way. The more I think this over, the more I am coming to the conclusion that we don't want a breaking change (major version) and that the functions should return the simple python objects in the way they do today. So, what we really need is just a way to get access to the raw One thing to expose is the I still need to think a but about exactly the right interface, but I everything you said has merit and I will try to implement an elegant solution. |
Issue:
Endpoint methods currently return the JSON result from the API as a python object.
This means all other information regarding the response is lost, such as the status code. It also means the return type is unknown as it can sometimes be a list and other times a dict.
This leads to the issue that the only way to determine if a response was a 400 or 200 is by inspecting the returned result.
Proposed solution:
Return response objects
with either thethe decoded json content is available withcontent
attribute decoded and replaced in place or with another attributeresult
with the decoded JSON.response.json()
I am happy to make a PR for this but it's a backwards incompatible change!
Perhaps there is a way to make it backwards compatible by returning an object of a custom class that behaves as the list/dict but with extra attributes for the response object. But this sounds complicated =]
The text was updated successfully, but these errors were encountered: