Skip to content

Deprecate and rename two timer overloads to interval #2975

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

Merged
merged 1 commit into from
Jun 17, 2015

Conversation

abersnaze
Copy link
Contributor

The existing six methods below didn't make sense and caused some confusion.

timer(delay, timeUnit[, scheduler]);  --> emits 0 after delay and completes
timer(delay, period, timeUnit[, scheduler]); --> emits 0 after delay and then i++ after every period forever
interval(period, timeUnit[, schduler); --> emits i++ after every period forever

I felt that the middle method acted more like the third method interval than first method timer. This PR is to make this change.

 timer(delay, timeUnit[, scheduler]);  --> emits 0 after delay and completes
+@Deprecated
 timer(delay, period, timeUnit[, scheduler]); --> emits 0 after delay and then i++ after every period forever
+interval(delay, period, timeUnit[, scheduler]); --> emits 0 after delay and then i++ after every period forever
 interval(period, timeUnit[, schduler); --> emits i++ after every period forever

PS: yes, I understand that we can't delete the deprecated timer method.
@davgross and if this PR is merged will have to change the images.

* the time unit for both {@code initialDelay} and {@code period}
* @return an Observable that emits a 0L after the {@code initialDelay} and ever increasing numbers after
* each {@code period} of time thereafter
* @see <a href="http://reactivex.io/documentation/operators/timer.html">ReactiveX operators documentation: Timer</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend leaving the documentation in place even when deprecating the method.

@abersnaze
Copy link
Contributor Author

Okay, I think I'm done changing the PR unless there are other comments.

@akarnokd
Copy link
Member

I'm not in favor of these changes.

@abersnaze
Copy link
Contributor Author

any particular reason?

@akarnokd
Copy link
Member

If it were only a matter of aliasing, I wouldn't see any problem, but you deprecate a historical method just because the name. Every use place need to be changed to avoid deprecation warnings in RxJava and outside it.

@abersnaze
Copy link
Contributor Author

I think it improved clarity of API and is directly related to a bug we had here. This PR already takes care of migrating the RxJava usages of timer to interval.

@akarnokd
Copy link
Member

The timer-interval confusion apparently is quite common so I'm going to merge this. Thanks for the changes.

akarnokd added a commit that referenced this pull request Jun 17, 2015
Deprecate and rename two timer overloads to interval
@akarnokd akarnokd merged commit 67f7988 into ReactiveX:1.x Jun 17, 2015
@abersnaze abersnaze deleted the interval branch June 17, 2015 14:24
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.

3 participants