-
Notifications
You must be signed in to change notification settings - Fork 544
Conversation
Hey @arkes! Sorry for the delayed response. This looks super great. Thanks for your contribution. @jonathankwok when you have a minute, can I get a second set of 👀 ? |
rescue ActiveUtils::ResponseError, ActiveShipping::ResponseError => e | ||
data = JSON.parse(e.response.body) | ||
|
||
RateResponse.new(false, data['error']['errorMessage'], data) |
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.
There is an assumption here that a parsed error body will always have a error:
key. This can raise a nil
error.
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 is very clean, well-done, and thorough! 👏 |
- Add URI - fix parse body with error message
endpoint = domestic_destination? ? @endpoints[:domestic] : @endpoints[:international] | ||
params = domestic_destination? ? domestic_params : international_params | ||
|
||
URI::HTTPS.build(host: HOST, path: endpoint, query: params.to_query).to_s |
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.
Nice. Way better.
Nice. Happy with it @arkes? Good to merge? |
Hey @kmcphillips, yes all good thanks! |
Released in 1.7.3 |
Support currently: