Skip to content

Conversation

@omus
Copy link
Member

@omus omus commented Apr 13, 2017

Allows user defined Period subtype to be compared against other Period subtypes.

See TimeZones.jl comment

@omus omus added the dates Dates, times, and the Dates stdlib module label Apr 13, 2017
@tkelman
Copy link
Contributor

tkelman commented Apr 13, 2017

thanks. could use non-methoderror tests of something this allows that doesn't work yet on master

@omus omus force-pushed the cv/period-compare branch from 26ae085 to 9501a71 Compare April 13, 2017 22:32
@JeffBezanson
Copy link
Member

Maybe it would work better to have TimePeriod and CalendarPeriod as abstract subtypes of Period, and use TimePeriod to mean what FixedPeriod means now? Whether a period corresponds to a fixed amount of time seems like a pretty important distinction. Then we wouldn't need methods that throw method errors.

@omus
Copy link
Member Author

omus commented Apr 17, 2017

FixedPeriod also contains Week and Day where as TimePeriod does not. Sometimes it isn't so clear when periods are fixed. In Base.Dates a Day is treated as a period of 24 hours however in TimeZones.jl a Day could mean 23, 24, or 25 hours depending on context.


# disallow comparing fixed to other periods
(==){T<:FixedPeriod, S<:OtherPeriod}(x::T, y::S) = throw(MethodError(==, (T, S)))
(==){T<:OtherPeriod, S<:FixedPeriod}(x::T, y::S) = throw(MethodError(==, (T, S)))
Copy link
Member

Choose a reason for hiding this comment

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

The MethodError should contain the arguments, not their types (when possible).

@JeffBezanson
Copy link
Member

Ah, I see. But by allowing Days == Seconds aren't we already implicitly declaring Day to be a fixed time period?

@omus
Copy link
Member Author

omus commented Apr 19, 2017

Currently Base does always treats a Day(1) == Hour(24). My example was just to show how periods people typically consider fixed may not be. Personally I'd like revise period comparison but that's out of the scope of this PR.

@JeffBezanson
Copy link
Member

Needs a rebase, then I guess we should merge this.

@omus omus force-pushed the cv/period-compare branch from 2eb38cb to e7391d9 Compare May 1, 2017 19:38
@omus
Copy link
Member Author

omus commented May 1, 2017

Rebased and squashed commits.

@JeffBezanson JeffBezanson merged commit ef02bc6 into master May 2, 2017
@omus omus deleted the cv/period-compare branch May 2, 2017 12:21
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request May 2, 2017
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request May 2, 2017
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request May 15, 2017
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dates Dates, times, and the Dates stdlib module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants