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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement hook to render specific types in OnNextValue #2632

Merged
merged 3 commits into from
Mar 17, 2015

Conversation

simonbasle
Copy link
Contributor

as discussed in #2468, allow implementations of RxJavaErrorHandler to define a rendering behavior for safe and known types to be rendered in the stacktrace of OnNextValue.

@simonbasle
Copy link
Contributor Author

FAILURE: Build failed with an exception.

  • Where:
    Build file '/home/travis/build/ReactiveX/RxJava/build.gradle' line: 27
  • What went wrong:
    A problem occurred evaluating root project 'rxjava'.

    No such property: release.scope for class: org.gradle.api.internal.project.DefaultProject_Decorated

@@ -150,6 +160,12 @@ private static String renderValue(Object value){
if (value instanceof Enum) {
return ((Enum<?>) value).name();
}

String pluggedRendering = RxJavaPlugins.getInstance().getErrorHandler().handleOnNextValueRendering(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the ErrorHandler should be statically referenced once at the top of this class?

@benjchristensen
Copy link
Member

This seems good to me. @akarnokd @zsxwing do you see any issues with this?

* @param item the last emitted item, that caused the exception wrapped in {@link OnErrorThrowable.OnNextValue}.
* @return a short {@link String} representation of the item if one is known for its type, or null for default.
*/
public final String handleOnNextValueRendering(Object item) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add @Experimental for right now during the 1.0.x release cycle?

@simonbasle
Copy link
Contributor Author

thanks @benjchristensen, I've implemented your suggestions

try {
return render(item);
} catch (Throwable t) {
return item.getClass().getName() + ERROR_IN_RENDERING_SUFFIX;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swallowing all Throwables may cause some issue. I suggest the following approach:

} catch (InterruptedException e) {
    Thread.currentThread().interrupt();
} catch (Throwable t) {
    Exceptions.throwIfFatal(t);    
}
return item.getClass().getName() + ERROR_IN_RENDERING_SUFFIX;

@simonbasle
Copy link
Contributor Author

have to get back to this, I brainfailed and didn't run the whole test suite. Turns out under this condition the reset of the plugin is just ignored since we'll have cached the default implementation when first loading OnNextValue class in some previous test. @benjchristensen was this just a readability suggestion or do you think I should try to correctly reset the static reference to the plugin?

benjchristensen added a commit that referenced this pull request Mar 17, 2015
Implement hook to render specific types in OnNextValue
@benjchristensen benjchristensen merged commit 020e843 into ReactiveX:1.x Mar 17, 2015
@simonbasle
Copy link
Contributor Author

awesome 😃 thanks @benjchristensen and everyone for your feedback

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.

3 participants