Conversation
Codecov Report
@@ Coverage Diff @@
## main #647 +/- ##
==========================================
- Coverage 84.97% 84.95% -0.03%
==========================================
Files 133 133
Lines 6728 6731 +3
==========================================
+ Hits 5717 5718 +1
- Misses 1011 1013 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
docarray/array/mixins/io/pushpull.py
Outdated
| if response.ok: | ||
| return response.json()['data'] | ||
| else: | ||
| if response.status_code == 403: |
There was a problem hiding this comment.
403 seems a magic number, lets use a constant
There was a problem hiding this comment.
@jina-ai/team-hubble @delgermurun @mapleeit these constants should come from hubble-sdk
There was a problem hiding this comment.
Does it make sense to use http.HTTPStatus.FORBIDDEN here?
It's standard http status code and libarary/http already provides that.
There was a problem hiding this comment.
The 403 error is not the only one. How about trying to get a message from every error response?
All errors should have a message, but checking for its existence is also helpful to have.
There was a problem hiding this comment.
Agree with @delgermurun
For all the status code >= 400, we should adopt the same strategy, which is printing the error message out.
| return response.json()['data'] | ||
| else: | ||
| if response.status_code >= 400 and 'readableMessage' in response.json(): | ||
| response.reason = response.json()['readableMessage'] |
There was a problem hiding this comment.
u should use HTTPStatus for somehing. There must be some constant from there, 400 seems magic to me
There was a problem hiding this comment.
I would've used it if it was an equal check (==).
I think it is more readable to use the actual number in comparison (it just means failed response).
The requests library does the same actually
Show the error message coming from the server in push, if the code is 403