-
Notifications
You must be signed in to change notification settings - Fork 72
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
Ivana - Scissors #66
base: main
Are you sure you want to change the base?
Ivana - Scissors #66
Conversation
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.
This looks great in general!
One opportunity for improvement for many people for this project is handling error codes from the API requests. In general, when writing code to call an API, it's appropriate to check whether the request was successful or it triggered some sort of error. This is where HTTP response codes come in handy! We can usually assume that a 2XX response code means there was no error and any other response code means there was some sort of error. It's very common to have an if...elif...else chain to handle different ranges of response codes.
def get_video(video_id): | ||
|
||
path = get_path(1) | ||
video = requests.get(path+f"{video_id}/").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.
This results in a crazy error message when there's a problem with the user input (such as an invalid id).
See my general feedback for more context on how this could be improved :)
For example:
Traceback (most recent call last):
File "/Users/jared/Developer/c15/video-store-cli/main.py", line 253, in <module>
main()
File "/Users/jared/Developer/c15/video-store-cli/main.py", line 249, in main
get_options()
File "/Users/jared/Developer/c15/video-store-cli/main.py", line 198, in get_options
print(delete_video(video_id))
File "/Users/jared/Developer/c15/video-store-cli/main.py", line 86, in delete_video
to_delete = get_video(video_id)
File "/Users/jared/Developer/c15/video-store-cli/main.py", line 56, in get_video
video = requests.get(path+f"{video_id}/").json()
File "/usr/local/lib/python3.9/site-packages/requests/models.py", line 900, in json
return complexjson.loads(self.text, **kwargs)
File "/usr/local/Cellar/[email protected]/3.9.0_5/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/__init__.py", line 346, in loads
return _default_decoder.decode(s)
File "/usr/local/Cellar/[email protected]/3.9.0_5/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/decoder.py", line 337, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/local/Cellar/[email protected]/3.9.0_5/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/decoder.py", line 355, in raw_decode
raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
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 Jared this is super helpful! I wasn't sure about how I should handle bad requests on this end. I wanted to alter the API code so errors like these would be handled by the original code instead of my local front end 😅 and this would definitely come in handy! thanks a lot :)
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! We can talk more about this in our 1:1 this week if it'd be useful!
No description provided.