Various Date/Time fixes #20226

quinnj · 2017-01-25T01:26:31Z

No description provided.

tkelman · 2017-01-25T01:29:04Z

you're deleting a lot of recent news additions here, probably wrong local conflict resolution?

tkelman · 2017-01-25T02:08:20Z

NEWS.md

@@ -219,15 +212,11 @@ Library improvements

  * `notify` now returns a count of tasks woken up ([#19841]).

+  * A new `Dates.Time` type was added that supports representing the time of day with up to nanosecond resolution.


should add PR reference

tkelman · 2017-01-25T02:13:10Z

base/dates/arithmetic.jl

-        ($op){T<:TimeType,P<:GeneralPeriod}(x::StridedArray{P}, y::T) = broadcast($op,x,y)
-        ($op){P<:GeneralPeriod}(y::TimeType, x::StridedArray{P}) = broadcast($op,x,y)
+        ($op){T<:TimeType}(x::AbstractArray{T}, y::GeneralPeriod) = broadcast($op, x, y)
+        ($op){P<:GeneralPeriod}(y::TimeType, x::StridedArray{P}) = broadcast($op, x, y)


shouldn't this one be addition-only? or be sure the argument orders match?

I think you rebased this one back

tkelman · 2017-01-25T02:38:02Z

base/dates/arithmetic.jl

 (+)(x::Date, y::Day)  = return Date(UTD(value(x) + value(y)))
 (-)(x::Date, y::Day)  = return Date(UTD(value(x) - value(y)))
 (+)(x::DateTime, y::Period) = return DateTime(UTM(value(x) + toms(y)))
 (-)(x::DateTime, y::Period) = return DateTime(UTM(value(x) - toms(y)))
 (+)(x::Time, y::TimePeriod) = return Time(Nanosecond(value(x) + tons(y)))
 (-)(x::Time, y::TimePeriod) = return Time(Nanosecond(value(x) - tons(y)))
 (+)(y::Period, x::TimeType) = x + y
-(-)(y::Period, x::TimeType) = x - y


I don't know who might be finding these useful, but it would be a bit less disruptive to phase it out with a warning in deprecated.jl.

I don't think anybody's relying on it.

It would seem crazy to think this would work, so it's hard to imagine relying on this. In other words, imo deleting this method is a bugfix not a feature deletion.

both #12094 and #12115 mention this which is why I pinged @GordStephen for input, but he might not be using julia as much lately, not sure

I would agree that this is crazy and more likely an oversight on my part than any kind of feature. I'm certainly not using this functionality.

tkelman · 2017-01-25T02:43:09Z

base/dates/periods.jl

    for p in y.periods
        x -= p
    end
    return x
 end
-(-)(x::CompoundPeriod,y::TimeType) = y - x
+(-)(x::CompoundPeriod, y::TimeType) = y - x


another instance of commutative subtraction that should be deprecated

(looks like @GordStephen may have been the author of a few of these)

likewise I think your rebase lost the removal of this

tkelman · 2017-01-25T06:30:29Z

base/dates/arithmetic.jl

+    if op == :+
+        @eval begin
+            ($op){T<:TimeType}(y::GeneralPeriod, x::AbstractArray{T}) = broadcast($op, x, y)
+            ($op){T<:TimeType,P<:GeneralPeriod}(x::StridedArray{P}, y::T) = broadcast($op, x, y)


since the argument order of this one is x, y on both lhs and rhs I think this one is fine to have defined for subtraction

tkelman · 2017-01-25T20:28:25Z

If you need a hand rebasing this, I can take a look late tonight. Just that one method to make a decision on. This addresses basically all of the feedback I had on the Time PR.

quinnj · 2017-01-25T20:35:43Z

I'll see if I can get to this; if not, feel free to go for it. I've been having computer issues and had to try and get my windows machine set back up for julia dev last night and had struggles.

quinnj · 2017-01-26T05:51:05Z

Rebasing now

… default step period

tkelman · 2017-01-26T06:50:31Z

base/dates/arithmetic.jl

@@ -86,11 +86,11 @@ for op in (:+, :-)
    @eval begin
        ($op){T<:TimeType}(x::AbstractArray{T}, y::GeneralPeriod) = broadcast($op, x, y)
        ($op){P<:GeneralPeriod}(y::TimeType, x::StridedArray{P}) = broadcast($op, x, y)


this one's bad though

wait, why is this one bad?

the argument order is y, x on the left and x, y on the right so it only works for commutative ops

quinnj · 2017-01-26T16:04:22Z

Travis is happy; @tkelman anything else to be changed here?

tkelman · 2017-01-26T17:52:04Z

L88 of base/dates/arithmetic.jl
L395 of base/dates/periods.jl

quinnj · 2017-01-26T18:08:51Z

@tkelman, could you fix the rebase for me? I unfortunately don't have access to my computer and my windows machine is all jacked. If you can't, I can probably get to it later tonight.

tkelman · 2017-01-26T20:10:14Z

Okay done. The dates tests fail if run with JULIA_CPU_CORES=1 but I don't think that's caused by this PR.

omus · 2017-01-26T20:14:56Z

test/dates/periods.jl

@@ -314,63 +314,62 @@ emptyperiod = ((y + d) - d) - y
 @test Dates.canonicalize(y - m + w - d + h - mi + s - ms).periods == Dates.Period[11m, 6d, 59mi, 999ms]
 @test Dates.canonicalize(-y + m - w + d - h + mi - s + ms).periods == Dates.Period[-11m, -6d, -59mi, -999ms]

-@test Dates.Date(2009,2,1) - (Dates.Month(1) + Dates.Day(1)) == Dates.Date(2008,12,31)
-@test (Dates.Month(1) + Dates.Day(1)) - Dates.Date(2009,2,1) == Dates.Date(2008,12,31)


Maybe this should be left in to ensure this behaviour throws an exception?

Good idea. Added back as @test_throws

ref https://github.com/JuliaLang/julia/pull/19920/files#r98093930

tkelman · 2017-01-27T14:47:44Z

base/dates/periods.jl

+(+)(a::TimeType, b::Period, c::Period) = (+)(a, b + c)
+(-)(a::TimeType, b::Period, c::Period) = (-)(a, b - c)
+(+)(a::TimeType, b::Period, c::Period, d::Period...) = (+)((+)(a, b + c), d...)
+(-)(a::TimeType, b::Period, c::Period, d::Period...) = (-)((-)(a, b - c), d...)


there's some strange associativity here too... if a - b - c is a - (b - c), then shouldn't a - b - c - d be a - (b - (c - d)) ?

tests pass without them, should we get rid of these subtractions too?

unless someone tells me otherwise

tkelman reviewed Jan 25, 2017

View reviewed changes

tkelman added the dates Dates, times, and the Dates stdlib module label Jan 25, 2017

tkelman reviewed Jan 25, 2017

View reviewed changes

tkelman added this to the 0.6.0 milestone Jan 25, 2017

quinnj mentioned this pull request Jan 25, 2017

Initial work to add a Time type to Base.Dates #12274

Merged

3 tasks

quinnj added 2 commits January 25, 2017 22:47

Add NEWS entry for the new Dates.Time type

665563b

Remove commutative subtraction methods. Fixes #20205

94aa901

quinnj added 4 commits January 25, 2017 22:51

Update Time adjuster function documentation to be more explicit about…

2dd9ff8

… default step period

Whitespace fixes for dates code

760e859

Add back one of the subtract methods

db31d92

Update NEWS again

48f1bc2

quinnj force-pushed the jq/timetocleanup branch from 4b6d77b to 48f1bc2 Compare January 26, 2017 06:16

tkelman reviewed Jan 26, 2017

View reviewed changes

Remove remaining commutative dates subtraction methods

e3268cd

omus reviewed Jan 26, 2017

View reviewed changes

Change tests of deleted methods to test_throws MethodError

8671e14

tkelman force-pushed the jq/timetocleanup branch from cf95a04 to 8671e14 Compare January 26, 2017 20:29

tkelman mentioned this pull request Jan 26, 2017

Remove problematic Dates conversions #19920

Merged

Comment out a test of a deprecated method

c8cabd5

ref https://github.com/JuliaLang/julia/pull/19920/files#r98093930

tkelman added 2 commits January 27, 2017 05:36

Merge branch 'master' into jq/timetocleanup

8a7a199

use f in signature of Time docstring to match body

8b35e72

tkelman reviewed Jan 27, 2017

View reviewed changes

Also remove questionably associative vararg subtraction

0fbc43e

unless someone tells me otherwise

tkelman merged commit 78448fa into master Jan 28, 2017

tkelman deleted the jq/timetocleanup branch January 28, 2017 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various Date/Time fixes #20226

Various Date/Time fixes #20226

quinnj commented Jan 25, 2017

tkelman commented Jan 25, 2017

tkelman Jan 25, 2017

tkelman Jan 25, 2017

tkelman Jan 26, 2017

tkelman Jan 25, 2017

quinnj Jan 25, 2017

StefanKarpinski Jan 26, 2017

tkelman Jan 26, 2017

GordStephen Jan 26, 2017

tkelman Jan 25, 2017

tkelman Jan 26, 2017

tkelman Jan 25, 2017

tkelman commented Jan 25, 2017

quinnj commented Jan 25, 2017

quinnj commented Jan 26, 2017

tkelman Jan 26, 2017

quinnj Jan 26, 2017

tkelman Jan 26, 2017

quinnj commented Jan 26, 2017

tkelman commented Jan 26, 2017 •

edited

Loading

quinnj commented Jan 26, 2017

tkelman commented Jan 26, 2017

omus Jan 26, 2017

tkelman Jan 26, 2017

tkelman Jan 27, 2017

tkelman Jan 27, 2017

quinnj Jan 27, 2017

		@@ -219,15 +212,11 @@ Library improvements

		* `notify` now returns a count of tasks woken up ([#19841]).

		* A new `Dates.Time` type was added that supports representing the time of day with up to nanosecond resolution.

Various Date/Time fixes #20226

Various Date/Time fixes #20226

Conversation

quinnj commented Jan 25, 2017

tkelman commented Jan 25, 2017

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

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

Choose a reason for hiding this comment

tkelman commented Jan 25, 2017

quinnj commented Jan 25, 2017

quinnj commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinnj commented Jan 26, 2017

tkelman commented Jan 26, 2017 • edited Loading

quinnj commented Jan 26, 2017

tkelman commented Jan 26, 2017

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

tkelman commented Jan 26, 2017 •

edited

Loading