Skip to content

Clarify contracts of CompositeSubscription in its javadoc #3458

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

Conversation

artem-zinnatullin
Copy link
Contributor

Current state of CompositeSubscription's javadoc — totally misleading 😄

Every time we need to store some subscriptions in it then clear them and leave CompositeSubscription in an operable state we have to read its sources because its actual behavior is not consistent with the javadoc.

CompositeSubscription.clear() leaves CompositeSubscription in operable state, when on the other hand CompositeSubscription.unsubscribe() sets isUnsubscribed = true and CompositeSubscription is not able to manage new subscriptions.

@akarnokd
Copy link
Member

👍

/**
* Unsubscribes itself and all inner subscriptions.
* <p>Notice that {@link CompositeSubscription} won't be able
* to manage new subscriptions after call of this method.
Copy link
Member

Choose a reason for hiding this comment

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

I feel this is unclear. You can still add Subscription to a CompositeSubscription after call of this method. How about After call of this method, new {@code Subscription}s added to {@link CompositeSubscription} will be unsubscribed immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fixed!

@artem-zinnatullin artem-zinnatullin force-pushed the CompositeSubscription-javadoc branch from 328ddbd to 4c33811 Compare October 17, 2015 22:35
@zsxwing
Copy link
Member

zsxwing commented Oct 18, 2015

👍

@akarnokd
Copy link
Member

Excellent!

akarnokd added a commit that referenced this pull request Oct 18, 2015
…-javadoc

Clarify contracts of CompositeSubscription in its javadoc
@akarnokd akarnokd merged commit 860e79f into ReactiveX:1.x Oct 18, 2015
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.

3 participants