Skip to content

Conversation

@samueltardieu
Copy link
Contributor

This fixes the issue in #346.

try {
scheduler.schedulePeriodicallyDirect(new CountingRunnable(), 1, -1, MINUTES);
fail();
} catch (IllegalArgumentException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be removed. A negative period makes no sense.

try {
worker.schedulePeriodically(new CountingRunnable(), 1, -1, MINUTES);
fail();
} catch (IllegalArgumentException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be removed. A negative period makes no sense.

Copy link
Contributor Author

@samueltardieu samueltardieu Nov 8, 2016

Choose a reason for hiding this comment

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

Where is this error message supposed to come from? Grepping RxAndroid and RxJava for "period < 0" gives no result. Moreover, wouldn't such test belong in RxJava?

I thought that these tests were disabled anyway, but I can reinstate them if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test is ignored because it's currently not implemented natively. That still doesn't change the fact that its removal is unrelated to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I will amend the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JakeWharton This has been amended, I don't know if you expect a ping, but in any case here it is.

@JakeWharton JakeWharton merged commit e628d1b into ReactiveX:2.x Nov 10, 2016
@JakeWharton
Copy link
Contributor

I'll do a point release tomorrow or Friday. Thanks!

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.

2 participants