types.jl: sleep with Date.Period types #19749

Shade5 · 2016-12-29T04:48:45Z

Supports sleep(Dates.Second(10))
sleep(Dates.Milliesecond(10))

Closes #19736

TotalVerb

Thanks for the contribution! A few comments:

As is, this will only work for seconds and milliseconds. That's what I asked for, but it can be made more generic. There's a Dates.toms function that takes any fixed-length Dates.Period and gives the number of milliseconds it represents. I would therefore suggest writing

sleep(time::Dates.Period) = sleep(Dates.toms(time) / 1000)

Also, in addition to sleep, there are functions like Timer, timedwait that could receive the same treatment.

I also think that this is not the right file to define these methods in. Could these methods be moved near the other methods for these functions?

As for tests, sleep is not easy to test properly. But I think having a test that the call does not raise an error is useful. One could do

@test sleep(Dates.Millisecond(10)) === nothing

Finally, make sure the file ends in a new line character. If it doesn't, it's likely that Travis will reject the PR.

Shade5 · 2016-12-29T05:26:59Z

@TotalVerb I'll make it more generic as you suggested and also include timer and timedwait.
I tried defining these new functions near the old ones but the problem is that while building julia the Date module is not defined yet while the time modules are being built. Thus I had to move it to the Date module

TotalVerb · 2016-12-29T05:29:20Z

You could consider moving the loading of Dates in sysimg.jl, line 357, a little earlier. Is there a reason why Dates is loaded so late? cc @quinnj

Shade5 · 2016-12-29T06:11:10Z

@TotalVerb I've added the changes you had suggested

TotalVerb · 2016-12-29T06:12:52Z

Note that the repeat argument for Timer is also a time period.

Shade5 · 2016-12-29T07:11:37Z

So, I'll have to add three more functions

Timer(time::Dates.Period, repeat::Real=0.0)
Timer(time::Real, repeat::Dates.Period=0.0)
Timer(time::Dates.Period, repeat::Dates.Period=0.0)

?

TotalVerb · 2016-12-29T07:14:03Z

I think it would be very confusing to mix real numbers and quantities with units. I would just define a single method for both arguments being Dates.Period.

Shade5 · 2016-12-29T08:19:30Z

Done!

tkelman · 2016-12-29T08:29:03Z

base/dates/types.jl

@@ -241,3 +241,8 @@ Base.isless(x::Date,y::Date) = isless(value(x),value(y))
 Base.isless(x::DateTime,y::DateTime) = isless(value(x),value(y))
 Base.isless(x::TimeType,y::TimeType) = isless(promote(x,y)...)
 ==(x::TimeType,y::TimeType) = ===(promote(x,y)...)
+
+import Base: sleep,Timer,timedwait
+sleep(time::Dates.Period) = sleep(Dates.toms(time) / 1000)


don't need Dates. while inside the Dates module

My bad, Fixed it

TotalVerb · 2016-12-29T10:44:28Z

base/dates/types.jl

+
+import Base: sleep,Timer,timedwait
+sleep(time::Period) = sleep(toms(time) / 1000)
+Timer(time::Period, repeat::Period=0.0) = Timer(toms(time) / 1000,toms(repeat) / 1000)


I don't think 0.0 will convert properly to Period. Use Second(0) as the default instead.

TotalVerb · 2016-12-29T10:45:31Z

base/dates/types.jl

+import Base: sleep,Timer,timedwait
+sleep(time::Period) = sleep(toms(time) / 1000)
+Timer(time::Period, repeat::Period=0.0) = Timer(toms(time) / 1000,toms(repeat) / 1000)
+timedwait(testcb::Function, time::Period, pollint::Float64) = timedwait(testcb, toms(time) / 1000, pollint)


pollint is supposed to be a keyword argument; it's also a time.

TotalVerb · 2016-12-29T22:22:50Z

base/dates/types.jl

+import Base: sleep,Timer,timedwait
+sleep(time::Period) = sleep(toms(time) / 1000)
+Timer(time::Period, repeat::Period=Second(0)) = Timer(toms(time) / 1000,toms(repeat) / 1000)
+timedwait(testcb::Function, time::Period, pollint::Period) = timedwait(testcb, toms(time) / 1000, toms(pollint) / 1000)


The signature is still not correct. Keyword arguments should be delimited like

timedwait(testcb::Function, time::Period; pollint::Period=Millisecond(100)) = ...

Try to match the existing signature,

function timedwait(testcb::Function, secs::Float64; pollint::Float64=0.1) ... end

TotalVerb · 2016-12-29T22:27:08Z

Please add some tests that these methods work properly. See test/channels.jl for a good way to test timedwait; you could repeat the existing test with a Period as argument instead. sleep and Timer can probably be sufficiently tested just by adding some lines to call them and making sure the methods are called properly.

Shade5 · 2016-12-30T08:21:29Z

@TotalVerb Where are the default tests for sleep and Timer? I've tried, but I can't find them.

TotalVerb · 2016-12-30T10:24:21Z

There are no tests specifically for sleep and Timer, but it should be enough to test the sleep(Dates.Millisecond(60)) version in threads.jl, replacing the existing sleep(0.06) calls.

Shade5 · 2016-12-30T10:47:33Z

Alright, I'll add the tests, there seems to be a problem with timedwait. I've posted it on gitter.

Shade5 · 2017-01-01T04:11:23Z

Added tests

TotalVerb · 2017-01-01T04:14:00Z

It would be better to avoid duplicating test code. Consider instead a for loop;

for period in (0.06, Dates.Millisecond(60))
    ... # testing code goes here, using `Timer(period)` / `sleep(period)`
end

Shade5 · 2017-01-01T06:50:48Z

I didn't add the test for timedwait in a forloop as the log output was different

TotalVerb · 2017-01-01T07:52:59Z

In that case, throw out the original test and keep only the new one. The Period version dispatches to the Real version anyway.

Shade5 · 2017-01-01T10:15:52Z

Anything else?

TotalVerb · 2017-01-01T22:25:18Z

test/channels.jl

    et=toq()
    # assuming that 0.5 seconds is a good enough buffer on a typical modern CPU
    try
        @assert (et >= 1.0) && (et <= 1.5)
        @assert !isready(rr3)
    catch
-        warn("timedwait tests delayed. et=$et, isready(rr3)=$(isready(rr3))")
+        warn("timedwait-Period tests delayed. et=$et, isready(rr3)=$(isready(rr3))")


No need for this -Period here.

TotalVerb · 2017-01-01T22:27:13Z

@tkelman I suppose since it is now past feature freeze, this will have to wait until 1.0, right?

Supports sleep(Dates.Second(10)) sleep(Dates.Milliesecond(10)) Closes JuliaLang#19736

Shade5 · 2017-01-02T04:41:11Z

Why is travis failing?

musm · 2017-01-02T04:47:49Z

test/channels.jl

@@ -92,7 +92,7 @@ end
    @async begin sleep(2.0); put!(rr3, :ok) end

    tic()
-    timedwait(callback, 1.0)
+    timedwait(callback, Dates.Second(1))


why is this necessary, shouldn't timedwait(callback, 1.0) still work?

As TotalVerb said, The Period version dispatches to the Real version anyway.

TotalVerb · 2017-01-02T08:14:51Z

Travis failure is #19792.

Shade5 · 2017-01-02T08:27:06Z

@TotalVerb Should I do anything?

Shade5 mentioned this pull request Dec 29, 2016

sleep, etc. with Dates.Period types #19736

Closed

TotalVerb suggested changes Dec 29, 2016

View reviewed changes

Shade5 force-pushed the dug branch from 4240a27 to 2e01a5b Compare December 29, 2016 06:09

Shade5 force-pushed the dug branch from 2e01a5b to 3d43a79 Compare December 29, 2016 08:18

tkelman reviewed Dec 29, 2016

View reviewed changes

Shade5 force-pushed the dug branch from 3d43a79 to abe3edb Compare December 29, 2016 08:54

TotalVerb suggested changes Dec 29, 2016

View reviewed changes

Shade5 force-pushed the dug branch from abe3edb to 03a6a04 Compare December 29, 2016 12:00

kshyatt added the dates Dates, times, and the Dates stdlib module label Dec 29, 2016

TotalVerb suggested changes Dec 29, 2016

View reviewed changes

Shade5 force-pushed the dug branch from 03a6a04 to 861ecf0 Compare January 1, 2017 04:11

Shade5 force-pushed the dug branch from 861ecf0 to fbd4878 Compare January 1, 2017 06:49

Shade5 force-pushed the dug branch from fbd4878 to 3dd6a79 Compare January 1, 2017 10:06

TotalVerb suggested changes Jan 1, 2017

View reviewed changes

types.jl: sleep with Date.Period types

4be146b

Supports sleep(Dates.Second(10)) sleep(Dates.Milliesecond(10)) Closes JuliaLang#19736

Shade5 force-pushed the dug branch from 3dd6a79 to 4be146b Compare January 2, 2017 03:17

musm reviewed Jan 2, 2017

View reviewed changes

StefanKarpinski approved these changes Jan 3, 2017

View reviewed changes

StefanKarpinski merged commit ea73c84 into JuliaLang:master Jan 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types.jl: sleep with Date.Period types #19749

types.jl: sleep with Date.Period types #19749

Shade5 commented Dec 29, 2016

TotalVerb left a comment •

edited

Loading

Shade5 commented Dec 29, 2016

TotalVerb commented Dec 29, 2016

Shade5 commented Dec 29, 2016

TotalVerb commented Dec 29, 2016 •

edited

Loading

Shade5 commented Dec 29, 2016

TotalVerb commented Dec 29, 2016

Shade5 commented Dec 29, 2016

tkelman Dec 29, 2016

Shade5 Dec 29, 2016

TotalVerb Dec 29, 2016

TotalVerb Dec 29, 2016

Shade5 Dec 29, 2016

TotalVerb Dec 29, 2016 •

edited

Loading

TotalVerb commented Dec 29, 2016 •

edited

Loading

Shade5 commented Dec 30, 2016

TotalVerb commented Dec 30, 2016

Shade5 commented Dec 30, 2016

Shade5 commented Jan 1, 2017

TotalVerb commented Jan 1, 2017

Shade5 commented Jan 1, 2017

TotalVerb commented Jan 1, 2017 •

edited

Loading

Shade5 commented Jan 1, 2017

TotalVerb Jan 1, 2017

Shade5 Jan 2, 2017

TotalVerb commented Jan 1, 2017

Shade5 commented Jan 2, 2017

musm Jan 2, 2017

Shade5 Jan 2, 2017

TotalVerb commented Jan 2, 2017

Shade5 commented Jan 2, 2017

types.jl: sleep with Date.Period types #19749

types.jl: sleep with Date.Period types #19749

Conversation

Shade5 commented Dec 29, 2016

TotalVerb left a comment • edited Loading

Choose a reason for hiding this comment

Shade5 commented Dec 29, 2016

TotalVerb commented Dec 29, 2016

Shade5 commented Dec 29, 2016

TotalVerb commented Dec 29, 2016 • edited Loading

Shade5 commented Dec 29, 2016

TotalVerb commented Dec 29, 2016

Shade5 commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

TotalVerb commented Dec 29, 2016 • edited Loading

Shade5 commented Dec 30, 2016

TotalVerb commented Dec 30, 2016

Shade5 commented Dec 30, 2016

Shade5 commented Jan 1, 2017

TotalVerb commented Jan 1, 2017

Shade5 commented Jan 1, 2017

TotalVerb commented Jan 1, 2017 • edited Loading

Shade5 commented Jan 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb commented Jan 1, 2017

Shade5 commented Jan 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb commented Jan 2, 2017

Shade5 commented Jan 2, 2017

TotalVerb left a comment •

edited

Loading

TotalVerb commented Dec 29, 2016 •

edited

Loading

TotalVerb Dec 29, 2016 •

edited

Loading

TotalVerb commented Dec 29, 2016 •

edited

Loading

TotalVerb commented Jan 1, 2017 •

edited

Loading