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

1.x: Add vararg of Subscriptions to CompositeSubscription. #3720

Merged

Conversation

klemzy
Copy link

@klemzy klemzy commented Feb 18, 2016

No description provided.

@akarnokd
Copy link
Member

Could you make sure you don't have that many space changes?

@klemzy
Copy link
Author

klemzy commented Feb 19, 2016

Sure, I will fix it.

@klemzy klemzy force-pushed the MultipleSubscriptionCompositeSubscription1x branch from 5ec27bf to 04f86ee Compare February 19, 2016 12:27
@klemzy
Copy link
Author

klemzy commented Feb 19, 2016

I have fixed formatting not to add indents on empty lines.

@@ -1,12 +1,12 @@
/**
* Copyright 2014 Netflix, Inc.
*
*
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 make sure these don't get changed?

@stevegury
Copy link
Member

  • squash your commit when you're done.

@akarnokd akarnokd changed the title Add vararg of Subscriptions to CompositeSubscription. 1.x: Add vararg of Subscriptions to CompositeSubscription. Mar 13, 2016
@@ -344,4 +344,121 @@ public void testAddingNullSubscriptionIllegal() {
csub.add(null);
}

@Test
public void testAddAll() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: test prefix is not needed, same for test below

@akarnokd
Copy link
Member

akarnokd commented Apr 2, 2016

Please rebase and squash.

@klemzy klemzy force-pushed the MultipleSubscriptionCompositeSubscription1x branch from 3f8f3ea to 114f50c Compare April 6, 2016 09:23
@akarnokd
Copy link
Member

This PR is a bit old. I'm 👍 with the API change but the test worries me too.

I'm merging this and will post a PR that hardens that test (+ renames them for @artem-zinnatullin sake :).

@akarnokd akarnokd merged commit 607f68c into ReactiveX:1.x Jun 17, 2016
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.

4 participants