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 ReplaySubject with infinite history #218

Merged
merged 9 commits into from
Apr 18, 2013
Merged

Implement ReplaySubject with infinite history #218

merged 9 commits into from
Apr 18, 2013

Conversation

johngmyers
Copy link
Contributor

No description provided.

@cloudbees-pull-request-builder

RxJava-pull-requests #64 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

RxJava-pull-requests #65 FAILURE
Looks like there's a problem with this pull request

Conflicts:
	rxjava-core/src/main/java/rx/operators/OperationTake.java
@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Member

I'm unavailable this week but will review this when I get back. I was focusing on getting schedulers done last week and wasn't able to get to this one.

@benjchristensen
Copy link
Member

I need to spend more time than I have right now to come up to speed on this code and what .Net does ... my focus has been on Schedulers (#19) still which hopefully is close (#235) but not quite done yet. Once Schedulers is done I'll turn my attention to this.

import rx.util.functions.Func1;

public class Subject<T> extends Observable<T> implements Observer<T> {
Copy link
Member

Choose a reason for hiding this comment

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

This guts the Subject class which should exist as it's own thing. See .Net for the equivalent: http://msdn.microsoft.com/en-us/library/system.reactive.subjects.subject(v=vs.103).aspx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then what do you suggest for the name of RxJava's analogue for Rx's ISubject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And you should see that I renamed Subject to PublishSubject

Copy link
Member

Choose a reason for hiding this comment

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

I guess the dilemma is ISubject vs Subject ... so what becomes what ...

Copy link
Member

Choose a reason for hiding this comment

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

Yes I see that @johngmyers ... I'm trying to figure out how to merge this and a couple other pull requests that all started overlapping. Apparently everyone wanted the same things at the same time :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning on doing ConnectableObservable after getting feedback on my approach so far. The other pull request doesn't deal with subjects other than the publish Subject. I would suggest you decline that other pull request until after you've figured out how you want to deal with Rx's ISubject.

@benjchristensen benjchristensen merged commit af383b5 into ReactiveX:master Apr 18, 2013
@benjchristensen
Copy link
Member

Please weigh in on this: #242

@johngmyers
Copy link
Contributor Author

I would appreciate some feedback on UnsubscribeTester. I left some comments in the commits above.

Note there were todo comments asking about desired semantics in the added unit tests.

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