-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Schedule tasks with negative delays immediately as per Scheduler spec #347
Conversation
scheduler.schedulePeriodicallyDirect(new CountingRunnable(), 1, -1, MINUTES); | ||
fail(); | ||
} catch (IllegalArgumentException e) { | ||
assertEquals("period < 0: -1", e.getMessage()); |
There was a problem hiding this comment.
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.
worker.schedulePeriodically(new CountingRunnable(), 1, -1, MINUTES); | ||
fail(); | ||
} catch (IllegalArgumentException e) { | ||
assertEquals("period < 0: -1", e.getMessage()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This fixes the issue in #346.
I'll do a point release tomorrow or Friday. Thanks! |
This fixes the issue in #346.