-
Notifications
You must be signed in to change notification settings - Fork 19
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
Implement ErrorReceivers support #45
Conversation
Putting up for early review and discussion of configuration, still needs tests. FYI @dlew who requested this feature |
This API would satisfy our needs just fine. My only concern is a bikeshedding one - that "TryOnError" doesn't really seem to evoke what it actually does (I'm not really sure the reasoning behind the name, even after thinking on it for a few minutes). |
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.
Agree with @dlew that name could be better. TryOnError
makes sense internally (we try to delegate to the onError
if supplied). I don't have a better name right now but i think the architecture looks good here. We can proceed with tests and figure out a name.
@@ -55,7 +55,11 @@ public void onSubscribe(Disposable d) { | |||
|
|||
@Override | |||
public void onError(Throwable e) { | |||
reportError(config, t, e, null); | |||
if (delegate instanceof TryOnError) { | |||
guardedDelegateCall(e2 -> reportError(config, t, e2, "onError"), () -> delegate.onError(e)); |
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.
I think we should just use the existing guardObserverCallbacks
check for onError as well.
Updated with the new naming + separated modified exception receiving after talking with @dlew. Still needs tests but I think this is where we want it to be API-wise |
rxdogtag/src/main/java/com/uber/rxdogtag/RxDogTagTaggedExceptionReceiver.java
Show resolved
Hide resolved
rxdogtag/src/main/java/com/uber/rxdogtag/RxDogTagTaggedExceptionReceiver.java
Show resolved
Hide resolved
This implements support for trying delegate onError calls if they opt in by implementing a new `TryOnError` interface. This will likely need to rely on guarded delegate call machinery, and need to figure out whether that should be another config or just supersede/defer to the existing config. Resolves #24
TryOnError -> RxDogTagErrorReceiver DeliverModifiedException -> RxDogTagModifiedExceptionReceiver
TryOnError -> RxDogTagErrorReceiver DeliverModifiedException -> RxDogTagModifiedExceptionReceiver
88cdfab
to
2547e31
Compare
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.
Looks good to me API wise
rxdogtag/src/main/java/com/uber/rxdogtag/RxDogTagTaggedExceptionReceiver.java
Show resolved
Hide resolved
rxdogtag/src/main/java/com/uber/rxdogtag/RxDogTagTaggedExceptionReceiver.java
Show resolved
Hide resolved
That last commit (82d5f26) is tests! |
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.
LGTM
This implements support for trying delegate onError calls if they opt in by implementing a new
RxDogTagErrorReceiver
interface. This can be useful for cases where they want RxDogTag's guarded delegate behavior even on onError() methodsThere is an extension of this interface called
RxDogTagTaggedExceptionReceiver
that can be used to mark when the implementer wants to receive RxDogTag's modified exceptions. These will never be checked by guarded delegate checks, and the user is expected to handle exceptions accordingly.Resolves #24