Fixes for non-ordered intervals #51
Conversation
|
Found one more change to make |
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
+ Coverage 98.23% 98.26% +0.02%
==========================================
Files 6 6
Lines 227 230 +3
==========================================
+ Hits 223 226 +3
Misses 4 4
Continue to review full report at Codecov.
|
src/anchoredinterval.jl
Outdated
| Base.first(interval::AnchoredInterval{P}) where P = min(interval.anchor, interval.anchor+P) | ||
| Base.last(interval::AnchoredInterval{P}) where P = max(interval.anchor, interval.anchor+P) | ||
| function Base.first(interval::AnchoredInterval{P}) where P | ||
| if P < zero(P) |
There was a problem hiding this comment.
I'd write this as a ternary I guess.
There was a problem hiding this comment.
I thought it was visually too busy with the +:
function Base.first(interval::AnchoredInterval{P}) where P
P < zero(P) ? interval.anchor + P : interval.anchor
endThere was a problem hiding this comment.
You could alternatively write it as interval.anchor + P * (P < zero(P)) 😛
There was a problem hiding this comment.
Also, we may want to just store the extrema as a tuple in the immutable type rather than performing this check every time. I think that would make the code cleaner too.
There was a problem hiding this comment.
This is more complicated than it first seems see: #52
There was a problem hiding this comment.
I'll go with a ternary statement with parameters around it. @ararslan's suggestion is good but there's an issue we need to solve with LaxZonedDateTimes which can result in the answer being wrong with you add a TimePeriod of 0...
There was a problem hiding this comment.
I was joking, really. Using a conditional, be it if or ternary, seems better to me.
| @@ -223,6 +223,10 @@ end | |||
|
|
|||
| ##### EQUALITY ##### | |||
|
|
|||
| function Base.:(==)(a::AnchoredInterval{P, T}, b::AnchoredInterval{P, T}) where {P, T} | |||
There was a problem hiding this comment.
Should we implement isequal here as well?
There was a problem hiding this comment.
We can safely fall back to isequal(::AbstractInterval, ::AbstractInterval) here. I'll add some tests though to make sure we have the proper behaviour.
src/anchoredinterval.jl
Outdated
| @@ -121,8 +121,14 @@ end | |||
|
|
|||
| ##### ACCESSORS ##### | |||
|
|
|||
| Base.first(interval::AnchoredInterval{P}) where P = min(interval.anchor, interval.anchor+P) | |||
| Base.last(interval::AnchoredInterval{P}) where P = max(interval.anchor, interval.anchor+P) | |||
| function Base.first(interval::AnchoredInterval{P}) where P | |||
There was a problem hiding this comment.
Don't forget to add a comment on why min and max won't work :)
|
Alright, I'm fine waiting to store the extrema in the type till later. LGTM. |
src/anchoredinterval.jl
Outdated
| @@ -121,6 +121,12 @@ end | |||
|
|
|||
| ##### ACCESSORS ##### | |||
|
|
|||
| # Note: For `first` and `last` the typically we would compute this is using `min` and `max`. | |||
There was a problem hiding this comment.
"We would typically compute first and last using min and max respectively, but we can get unexpected behaviour if adding the span to the endpoint produces a value that is no longer comparable (e.g., NaN)."?
No description provided.