Introduce ApolloExceptions #379
Conversation
sav007
requested review from
brianPlummer,
BenSchwab,
digitalbuddha and
marwanad
|
|
||
| @Nonnull ApolloPrefetch enqueue(@Nullable Callback callback); | ||
|
|
||
| ApolloPrefetch clone(); | ||
|
|
||
| void cancel(); | ||
|
|
||
| interface Callback { | ||
| void onSuccess(); | ||
| abstract class Callback { |
There was a problem hiding this comment.
Nice, like the option to use a single callback and inheritance, or the split methods.
|
|
||
| public abstract void onFailure(@Nonnull ApolloException e); | ||
|
|
||
| public void onHttpError(@Nonnull ApolloHttpException e) { |
There was a problem hiding this comment.
I think it would be nice to pass in the status code and response here, making this look like:
public void onNetworkError(int status, Response response, @Nonnull ApolloNetworkException e)I fell it is nice not having to dig through the exception, secondly, it makes it clearer what we have to work with
There was a problem hiding this comment.
Not sure about this, as it becomes too clumsy to have 3 params to handle one error.
And if user really decides to handle network error then don't see any issue to dig into ApolloNetworkException to extract what he needs.
| public abstract void onFailure(@Nonnull ApolloException e); | ||
|
|
||
| public void onHttpError(@Nonnull ApolloHttpException e) { | ||
| onFailure(e); |
There was a problem hiding this comment.
Thoughts on calling closeQuietly(e.response()) after we call onFailure? I think it would prevent a lot of leaked connections
| onFailure(e); | ||
| } | ||
|
|
||
| public void onNetworkError(@Nonnull ApolloNetworkException e) { |
There was a problem hiding this comment.
Should we have the exception ApolloNetworkException? I think an IOException is more fitting for this case
There was a problem hiding this comment.
We decided to wrap with ApolloNetworkException that extends ApolloException.
So from our sync API call (com.apollographql.apollo.ApolloCall#execute) user should expect only one type of errors ApolloException.
Closes #367
Introduces 4 ApolloExceptions:
ApolloExceptiongeneric any runtime errors should be wrapped into itApolloHttpExceptionif http status code != 200ApolloNetworkExceptionany network, timeouts etc.ApolloParseExceptionresponse parse errorsSee new API:
https://github.com/apollographql/apollo-android/pull/379/files#diff-18999fee7e5f70c2f862624cbab6d833
https://github.com/apollographql/apollo-android/pull/379/files#diff-b059ebbc703ddbbde44ba44eed598837
@mandrizzle