Skip to content

Optional constructor rounding#3

Open
omus wants to merge 2 commits into
masterfrom
cv/round
Open

Optional constructor rounding#3
omus wants to merge 2 commits into
masterfrom
cv/round

Conversation

@omus

@omus omus commented Mar 5, 2018

Copy link
Copy Markdown
Collaborator

Only round an AnchoredInterval when the keyword round is set. Display of AnchoredIntervals using Period spans will always show the interval as rounded currently

@codecov

codecov Bot commented Mar 5, 2018

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #3   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         145    140    -5     
=====================================
- Hits          145    140    -5
Impacted Files Coverage Δ
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 99900f2...be3707b. Read the comment docs.

@spurll

spurll commented Mar 5, 2018

Copy link
Copy Markdown
Contributor

The issue that I have with this approach is that it allows the user to pass in a round=true kwarg that doesn't actually do something when P isn't a Period. I think a cleaner solution is to just create a pair of pseudoconstructors HE and HB that do the rounding. (I've done this in the branch that I'm working on now, but am still working a fix for the display of non-rounded AnchoredIntervals.)

@omus

omus commented Mar 5, 2018

Copy link
Copy Markdown
Collaborator Author

We could also support rounding for non-Period types

@spurll

spurll commented Mar 5, 2018

Copy link
Copy Markdown
Contributor

Yep. The syntax would be slightly different (using the value for Period and the type for everything else). I no longer think it's worth implementing for the type itself, though, given that our only use case is for hour ending and we were talking about having a shorter pseudoconstructor anyway.

@omus

omus commented Mar 6, 2018

Copy link
Copy Markdown
Collaborator Author

Ok, I expect I'll close this. I'll leave it open for now as things change around this.

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.

2 participants