SchedulePeriodically Signature

[benjchristensen]

This pull requests adds TimeUnit delayUnit to the signature:

public Subscription schedulePeriodically(final Action1<Scheduler.Inner> action, long initialDelay, TimeUnit delayUnit, long period, TimeUnit periodUnit)

This was derived from feedback from @headinthebox while writing the Scala APIs.

Reason for ...

• it better matches languages that have a single object that represent both together such as Scala.
• Java 8 adds Duration: http://download.java.net/jdk8/docs/api/java/time/Duration.html

Reason against ...

• Java 7 and earlier doesn't do it this way on their Executor.
• Java 8 despite adding Duration still doesn't use it on their ScheduledExecutorService

Thoughts, concerns, opinions?

[cloudbees-pull-request-builder]

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

[benjchristensen]

I don't know that this change is worth pursuing and being different than Executor. It's easy enough to use as is and convert from languages like Scala (as it's wrapper already does). Some day (maybe v2.0) when we support Java 8 as the minimum we could then use Duration initial, Duration period.

Agreement? Disagreement? Evidently I'm not strongly opinionated either way on this one so feedback would be helpful.

@headinthebox Since you are a proponent for this change, please weigh in.

[headinthebox]

Why not have two overloads? That should work just fine
I find it a bit (extremely) distasteful to have one unit for two totally unrelated durations.

[akarnokd]

Using overload is a safe way, but we need to add overloads to operators which take 2 durations (such as Timer, Buffer and Window).

[headinthebox]

but we need to add overloads to operators which take 2 durations (such as Timer, Buffer and Window).

Yes, I mentioned that earlier.

[benjchristensen]

Only because we have been trying to eliminate overloads. An overload of this one feels like it means we can't decide.

[headinthebox]

In that case, my preference is clear ;-)

[benjchristensen]

@akarnokd Should we merge this change and adopt the new signature, or stick with the Java ScheduledExecutorService signature? The other schedule methods don't match it, so it's not a strong reason to stay matching.

@abersnaze I believe you are in favor of merging this change, correct?

[akarnokd]

I'd rather chose a common unit here instead of converting to nanos.

TimeUnit common = initialDelayUnit.compareTo(periodUnit) < 0 ? initialDelayUnit : periodUnit;

final long initial = common.convert(initialDelay, initialDelayUnit);
final long period = common.convert(period, periodUnit);

[benjchristensen]

The common unit is nanos. It already was before this change.

[neerajrj]

My preference would be to stick with the Java convention. Ideal thing would be to have a single object encapsulating the delay unit and time but that is not an option currently.

[akarnokd]

Isn't there a scala wrapper anymore where this could be shifted over?

I would support the new split syntax only if the current overload is kept with deprecated annotation until the time of the major purge.

[benjchristensen]

Isn't there a scala wrapper anymore where this could be shifted over?

Yes, the Scala wrapper already does it this way.

[benjchristensen]

I'm closing this out, the reasoning for doing it is not strong enough as languages like Scala can already deal with it whereas in Java the way it is currently is idiomatic.