-
Notifications
You must be signed in to change notification settings - Fork 429
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
Use DispatchTimeInterval to offset Date values #48
Conversation
This gives us improved expressiveness when specifying time offsets in the scheduler
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.
👍 looking good
} | ||
|
||
extension DispatchTimeInterval { | ||
internal func asTimeInterval() -> TimeInterval { |
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 would be better as a derived property instead of a function.
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.
@NachoSoto would you keep the asTimeInterval
name? I've not seen precedent for this pattern yet outside of Apple's use of cgColor
(i.e. using the type name itself) except it'd be strange to call a DispatchTimeInterval
method called timeInterval
. 😛
/// - returns: Scaled interval in microseconds | ||
internal static func *(lhs: DispatchTimeInterval, rhs: Double) -> DispatchTimeInterval { | ||
let seconds = lhs.asTimeInterval() * rhs | ||
return .microseconds(Int(seconds * 1000 * 1000)) |
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 is falling into one of the points I wanted to fix with this approach: potential overflows.
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.
@NachoSoto So this is only used internally to "scale down" a time interval. Specifically it's used only to scale intervals to 10% of their original value for the default leeway parameter.
But you are correct. If seconds is over ~200,000, 10% is 2000, and hence we end up with ~2billion which will overflow a signed integer on 32-bit platforms (of which there aren't really any more for much longer, anyway…)
But I'm pretty sure that even 200k seconds is going to be a rarely (if ever) specified interval for our APIs.
One of the tests I plan to add is to find this tipping point.
@@ -57,7 +99,7 @@ public protocol DateSchedulerProtocol: SchedulerProtocol { | |||
/// - returns: Optional `Disposable` that can be used to cancel the work | |||
/// before it begins. | |||
@discardableResult | |||
func schedule(after date: Date, interval: TimeInterval, leeway: TimeInterval, action: @escaping () -> Void) -> Disposable? |
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.
Awesome. Can you add the deprecations for all these?
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.
I added them all in the "deprecations bucket file."
@NachoSoto I'm pretty sure I addressed everything you pointed out. Let me know if I missed anything (or created any new issues.) Removing [WIP] tag… |
According to https://github.com/AlwaysRightInstitute/CDispatch, there should no longer be a need to reference CDispatch.
@NachoSoto Everything look OK to you for this to go in? Any others want to take a look? @mdiep, @andersio? The related issue (#33) is the last item on the 1.0 milestone. |
I'll try to take a look in the next few days unless someone beats me to it. I did put #22 in the 1.0 milestone just now because I think that needs to be addressed. |
Brilliant. Thanks @liscio! |
This work addresses #33.
There are still things to do before I'll pull the WIP status…
Date
/DispatchTimeInterval
*
operatorDispatchTimeInterval
conversions