Skip to content

Making RxPlugins reset() public #3820

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

Merged
merged 1 commit into from
Apr 29, 2016

Conversation

shivangshah
Copy link

Discussions found here: #2297

@ZacSweers
Copy link
Contributor

👍

@stevegury
Copy link
Member

Based on the discussion, it's definitely helpful, but also dangerous.
Could you add a comment indicating that it's dangerous?

Otherwise 👍

@artem-zinnatullin
Copy link
Contributor

Yeah, same concern about dangerous that this method may add.

It looks like an easy way to do testing but in multithreaded environment (integrational or functional tests for example) it may break a lot of things in "random" manner.

@stevegury @akarnokd let's also mark it as @Experimental so we'll be able to remove it if community will have more issues than benefits?

@stevegury
Copy link
Member

Yeah, good idea for the @Experimental flag

@shivangshah shivangshah force-pushed the rxplugins-reset-public branch from dfa1b68 to 6cbbef4 Compare April 7, 2016 05:27
@shivangshah
Copy link
Author

Thanks for the feedback @stevegury , @artem-zinnatullin. Just added the @experimental tag. Comments in the PR + commit message.

@stevegury
Copy link
Member

Please add a javadoc block, and explicitly mention that resetting the plugins is dangerous/unsafe during application runtime (I suspect most people won't check the link), keep the link at the end.

@JakeWharton
Copy link
Contributor

@shivangshah Friendly ping. Please update with Javadoc.

@shivangshah
Copy link
Author

@JakeWharton yes .. thanks for the reminder this totally slipped my mind .. Let me do it now ..

Discussions found here: ReactiveX#2297
Adding @experimental tag because exposing reset() could be dangerous
Adding javadocs for `reset()` API.
Explicitly mentioning how caution is advised when using `reset()` API.
Also mentioning links to detailed discussions on github issue.
@shivangshah shivangshah force-pushed the rxplugins-reset-public branch from 6cbbef4 to a2416ca Compare April 22, 2016 20:51
@shivangshah
Copy link
Author

@JakeWharton : Done ! Let me know if we need to add anything else as a part of the documentation.

@akarnokd
Copy link
Member

👍

@stevegury
Copy link
Member

👍

@akarnokd akarnokd merged commit 89fd2dd into ReactiveX:1.x Apr 29, 2016
shivangshah pushed a commit to shivangshah/spring-cloud-netflix that referenced this pull request May 5, 2016
This update on RxJava version is in regards to the PR that got merged [`here`](ReactiveX/RxJava#3820) for making RxJavaPlugins' `reset` method to be public. We have to work this around in Spring Sleuth. Details on the implementation can be found [`here`] (spring-cloud/spring-cloud-sleuth#243).

1.1.5
spencergibb pushed a commit to spring-cloud/spring-cloud-netflix that referenced this pull request May 6, 2016
This update on RxJava version is in regards to the PR that got merged [`here`](ReactiveX/RxJava#3820) for making RxJavaPlugins' `reset` method to be public. We have to work this around in Spring Sleuth. Details on the implementation can be found [`here`] (spring-cloud/spring-cloud-sleuth#243).

1.1.5
spencergibb added a commit to spring-cloud/spring-cloud-netflix that referenced this pull request May 6, 2016
* pull1007:
  Updating RxJava version to 1.1.5 This update on RxJava version is in regards to the PR that got merged [`here`](ReactiveX/RxJava#3820) for making RxJavaPlugins' `reset` method to be public. We have to work this around in Spring Sleuth. Details on the implementation can be found [`here`] (spring-cloud/spring-cloud-sleuth#243).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants