Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Shopify.Error to handle response failures #38

Merged
merged 1 commit into from
Jun 4, 2018

Conversation

vladimir-e
Copy link
Contributor

@vladimir-e vladimir-e commented May 30, 2018

@Ninigi thanks for a quick feedback on #37

I think returning {:error, %Shopify.Error{}} would bubble up, so guess your approach would work 馃憤
We would have to put some thought into the error fields, since we might want to use it for the errors returned by the shopify api as well

I think we should keep returning shopify errors wrapped in Shopify.Response, because it is response.

And for failed requests we pass errors from HTTPoison. Actually HTTPoison does the same, passing errors from hackney. So with the solution I propose in this PR we get ability to handle these errors coming from the deep.

we would need a data and a code field to make it backwards compatible.

If we don't use Shopify.Error for API errors we don't really need to make it backward compatible.

I tested this solution on my real app that exports thousands of records, by switching network on and off to produce network errors, and it worked well.

@Ninigi Ninigi self-assigned this May 31, 2018
@Ninigi
Copy link
Collaborator

Ninigi commented May 31, 2018

Thanks, will look into this on Sunday :)

@Ninigi
Copy link
Collaborator

Ninigi commented Jun 4, 2018

closes #37

@Ninigi Ninigi closed this Jun 4, 2018
@Ninigi Ninigi reopened this Jun 4, 2018
@Ninigi Ninigi merged commit ac03439 into nsweeting:master Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants