Skip to content
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

Move away from using span value as a type parameter #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

omus
Copy link
Collaborator

@omus omus commented Mar 8, 2018

Moving away from using the span value as a type parameter as this does not allow us to check properties of the type like "ending" (span is < 0) or "beginning" (span is > 0).

@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         177    188   +11     
=====================================
+ Hits          177    188   +11
Impacted Files Coverage Δ
src/endpoint.jl 100% <ø> (ø) ⬆️
src/description.jl 100% <100%> (ø) ⬆️
src/anchoredinterval.jl 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4d716a...cc6f3df. Read the comment docs.

@omus
Copy link
Collaborator Author

omus commented Mar 8, 2018

The current commit switches AnchorInterval{P, T} to AnchorInterval{T, P} in preparation for additional commits which end up changing AnchorInterval to its final form of AnchorInterval{T, S, E} where S is the type of the span and E is an instance of Direction (either Ending or Beginning) which indicates the direction of the endpoint.

@omus
Copy link
Collaborator Author

omus commented Mar 8, 2018

Currently the span can field can be positive/negative which is strange as the span accessor only returns a positive value. I believe the code can be simplified if we restrict the span parameter passed into AnchoredInterval to be always positive.

@omus
Copy link
Collaborator Author

omus commented Mar 8, 2018

Note that with these changes since the value of the span is no longer part of the type things like HourEnding have spans of Hour but not necessarily one hour:

julia> HourEnding(now(), Hour(3))
HourEnding{DateTime}(2018-03-08T14:44:00.085, 3 hours, Inclusivity(false, true))

This does have benefits as it makes our intersect function always return the same type:

julia> dt = now()
2018-03-08T14:44:46.966

julia> intersect(IntervalEnding(dt, Hour(3)), HourEnding(dt - Hour(1), Hour(3)))
HourEnding{DateTime}(2018-03-08T13:44:46.966, 2 hours, Inclusivity(false, true))

julia> ans isa IntervalEnding{DateTime, Hour}
true

@omus omus changed the title WIP: Move away from using span value as a type parameter Move away from using span value as a type parameter Mar 8, 2018
@spurll
Copy link
Contributor

spurll commented Mar 8, 2018

I am eying this PR with a mix of excitement and trepidation. I look forward to reviewing this at a later date.

@omus
Copy link
Collaborator Author

omus commented Mar 8, 2018

Fair enough. I would like to get this merge next week sometime so let me know if you want to go through it together.

@spurll
Copy link
Contributor

spurll commented Mar 9, 2018

That would be good. I'm not sure I'll have time next week, but I'll let you know.

@omus omus force-pushed the cv/interval-direction branch 2 times, most recently from f36374c to 27aad6e Compare March 10, 2018 01:01
@omus omus force-pushed the cv/interval-direction branch 3 times, most recently from eeb02a4 to ba4c908 Compare March 10, 2018 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants