Change onError(Exception) to onError(Throwable) - Issue #296

[benjchristensen]

This changes Observer#onError(Exception e) to Observer#onError(Throwable e)

See "Observer#onError should use Throwable" #296 for discussion that led to this change.

This is a breaking change and will result in a version bump from 0.9.x to 0.10.x.

[cloudbees-pull-request-builder]

RxJava-pull-requests #190 SUCCESS
This pull request looks good

[mttkay]

Perhaps just replace this block with @see onErrorResumeNext? It still refers to the former method rather than onExceptionResumeNext which might be confusing to read.

[benjchristensen]

The only part of it that wasn't changed to be specific is the image: onErrorResumeNext.png

I haven't had time to create a new marble diagram but that will come. Most likely @DavidMGross will beat me to it before I get to it.

The rest of the comment is important I feel as it explains the difference from onErrorResumeNext:

... if it encounters an error of type {@link java.lang.Exception}.
This differs from {@link #onErrorResumeNext} in that this one does not handle {@link java.lang.Throwable} or {@link java.lang.Error} but lets those continue through.

[codefromthecrypt]

diff isn't easy to unveil pitfalls, as catch (Throwable e) diffs don't have a large enough window to show whether or not error propagation occurs after signaling.

Might be worthwhile doing a visual scan via reference check or string search in the ide where catch (Throwable e) to see if errors are propagated after signaling or not. We could possibly assume that a catch (Throwable e) conversion is more likely to imply a change in the next line or two than not... Not sure.

[codefromthecrypt]

another option would be to make static utility methods like guava's Throwables.propagate and use in common cases such as coercing to Exception, RuntimeException, or propagating errors.

ex.

catch (Throwable e) {
  thingThatTakesException.signal(asException(e));
  propagateIfError(e);
}

[benjchristensen]

@adriancole

diff isn't easy to unveil pitfalls, as catch (Throwable e) diffs don't have a large enough window to show whether or not error propagation occurs after signaling.

This change is very bad for reviewing with a diff that only include a couple lines on either side. When I review the 60+ files before submitting the pull request I had to keep flipping back to the full source code to read the context.

It's just the nature of the change ...

[cloudbees-pull-request-builder]

RxJava-pull-requests #191 SUCCESS
This pull request looks good

[benjchristensen]

@adriancole

another option would be to make static utility methods like guava's Throwables.propagate and use in common cases such as coercing to Exception, RuntimeException, or propagating errors.

This already exists in RxJava (Exceptions.propagate(Throwable)), but I just updated it to also throw Error now as I'd missed doing that change last night: benjchristensen@87e308a#L0L25

[cloudbees-pull-request-builder]

RxJava-pull-requests #192 SUCCESS
This pull request looks good

[codefromthecrypt]

Cool sounds like an easy adjustment then!

On Thursday, August 1, 2013, Ben Christensen wrote:

@adriancole https://github.com/adriancole

another option would be to make static utility methods like guava's
Throwables.propagate and use in common cases such as coercing to Exception,
RuntimeException, or propagating errors.

This already exists in RxJava (Exceptions.propagate(Throwable)), but I
just updated it to also throw Error now as I'd missed doing that change
last night: benjchristensen/RxJava@87e308a#L0L25benjchristensen@87e308a#L0L25

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/315#issuecomment-21952181
.

[cloudbees-pull-request-builder]

RxJava-pull-requests #193 SUCCESS
This pull request looks good

[benjchristensen]

Moving forward with the merge ...