Skip to content

Conversation

@ruippeixotog
Copy link
Contributor

This pull request adds a wrapper for JavaRx UnicastSubject. Although the Subject is experimental, I would like to see a Scala wrapper for it as it's very useful in one of my projects.

I read #100, but I'm still not sure of how I should mark classes as experimental. I opted to use the annotation that is used in Java, but please tell me if you want it another way and I'll change it.

Copy link
Collaborator

@samuelgruetter samuelgruetter left a comment

Choose a reason for hiding this comment

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

Thanks for contributing 😃
I have just one minor scaladoc nitpick (see below), otherwise I like it 👍

* requests a limited amount, queueing is involved and only those values are retained which
* weren't requested by the `Subscriber` at that time.
*/
@Experimental
Copy link
Collaborator

Choose a reason for hiding this comment

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

Putting the @Experimental annotation here is fine, but in addition, you could also add a red badge in the scaladoc comment, as in ErrorDelayingObservable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, it looks fine!

Copy link
Member

Choose a reason for hiding this comment

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

@ruippeixotog you forgot to add $experimental to the class doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I also added @Experimental to the methods in order to be consistent with what was done in ErrorDelayingObservable.

@ruippeixotog ruippeixotog force-pushed the unicast-subject branch 2 times, most recently from 3494dba to 55aff5e Compare October 18, 2016 08:54
Copy link
Collaborator

@samuelgruetter samuelgruetter left a comment

Choose a reason for hiding this comment

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

LGTM

@zsxwing
Copy link
Member

zsxwing commented Oct 18, 2016

👍

@zsxwing zsxwing merged commit 5399016 into ReactiveX:0.x Oct 18, 2016
@zsxwing
Copy link
Member

zsxwing commented Oct 18, 2016

Thanks @ruippeixotog

@ruippeixotog ruippeixotog deleted the unicast-subject branch November 3, 2016 00:50
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