-
Notifications
You must be signed in to change notification settings - Fork 372
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
Add new Origin Trials API #3214
Conversation
Co-authored-by: James C Scott III <[email protected]>
e1d4bb8
to
fa90f27
Compare
950e2cf
to
cd6f1b3
Compare
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.
The frameworks/origin_trials_client.py should not have knowledge of the type of http status codes or message to return to the frontend. Instead of returning a tuple of value and error, you need to raise an error.
https://google.github.io/styleguide/pyguide.html#244-decision
Then in api/origin_trials_api.py, you can do things like:
try:
trials_list = origin_trials_client.get_trials_list()
except ExceptionType1:
self.abort(500, 'Error obtaining origin trial data from API')
except ExceptionType2:
self.abort(500, 'Malformed response from origin trials API')
Regarding creating your own Exception types vs re-raising existing ones: I'm okay re-raising the existing ones for now. We can always refactor that later on.
framework/origin_trials_client.py
Outdated
f'{settings.OT_API_URL}/v1/trials', | ||
params={'prettyPrint': 'false', 'key': key}) | ||
response.raise_for_status() | ||
except requests.exceptions.HTTPError: |
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.
You should either re-raise this exception or create your own exception like class FailedToGetDataError(requests.exceptions.HTTPError)
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 want to make sure I'm doing this right - should I not be passing the error info in the return value and instead be returning an actual error? Or am I raising the error in the get_trials_list()
function and then catching it in the origin_trials_api.do_get()
to raise it again there?
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.
You'll do this part Or am I raising the error in the get_trials_list() function and then catching it in the origin_trials_api.do_get()
.
Then in do_get(), it will catch that specific exception and call the right abort:
try:
trials_list = origin_trials_client.get_trials_list()
except ExceptionType1: # the http one
self.abort(500, 'Error obtaining origin trial data from API')
except ExceptionType2: # The valueerror one
self.abort(500, 'Malformed response from origin trials API')
framework/origin_trials_client.py
Outdated
|
||
response_json = response.json() | ||
if 'trials' not in response_json: | ||
return [], (500, 'Malformed response from origin trials API') |
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.
Same as above. But the error could be ValueError
or a new class error based on ValueError
.
framework/origin_trials_client.py
Outdated
if key == None: | ||
return [], None | ||
|
||
response = requests.get( |
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.
You should still have the try-except for the 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.
Ah, I wasn't sure if I was supposed to leave the try/except block in here along with the one in the origin_trials_api
function
framework/origin_trials_client.py
Outdated
# Data type for information if an error occurred. Error code + message. | ||
ERROR_INFO_TYPE = tuple[int, str]|None |
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.
# Data type for information if an error occurred. Error code + message. | |
ERROR_INFO_TYPE = tuple[int, str]|None |
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.
Sorry, I remember removing the code for the error returns and the type hinting, but it doesn't show up here. It might have been part of an abandoned change. Either way, I should have caught it before sending it over for re-review!
Co-authored-by: James C Scott III <[email protected]>
Co-authored-by: James C Scott III <[email protected]>
Co-authored-by: James C Scott III <[email protected]>
Co-authored-by: James C Scott III <[email protected]>
270de4b
to
6c898e7
Compare
This PR adds the new Origin Trials API that will be used to interact with the OT API on the server side. A new endpoint is also created to query the OT API for origin trial data.