between supports Date/IDate with missing bounds#7751
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7751 +/- ##
=======================================
Coverage 98.85% 98.85%
=======================================
Files 87 87
Lines 17121 17127 +6
=======================================
+ Hits 16925 16931 +6
Misses 196 196 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # errors name the bound that failed coercion | ||
| x = as.Date("2020-01-01") + 0:4 | ||
| test(2038.39, between(x, "not-a-date", "2020-01-04"), error="Date while 'lower' was not, coercion to Date failed") | ||
| test(2038.40, between(x, "2020-01-02", "not-a-date"), error="Date while 'upper' was not, coercion to Date failed") |
There was a problem hiding this comment.
not sure we need both and 2038.39 and 2038.40
| x[InRange == FALSE, c("RangeBegin", "RangeEnd") := list(Date + 1L, Date + 2L)] | ||
| test(2038.36, x[Active == TRUE & between(Date, RangeBegin, RangeEnd), Date], as.Date(c("2020-01-01", "2020-01-05"))) | ||
| # Date character bounds coercion | ||
| test(2038.37, between(x$Date, "2020-01-02", "2020-01-04"), c(FALSE, TRUE, TRUE, TRUE, FALSE, FALSE)) |
There was a problem hiding this comment.
This test does not fail pre the PR, so its probably covering the same lines (just food for thought)
ben-schwen
left a comment
ben-schwen
left a comment
There was a problem hiding this comment.
code change LGTM. some minor things about the tests.
|
@ben-schwen thanks for the review! I adopted your suggestion for the test |
|
TY! |
2 participants
Closes #7281
I believe this implementation matches @aitap's suggestion, where Date/IDates are passed to the C code directly. I tried to line up the character bound parsing with the existing implementation for
POSIXct.This is my first time contributing to
data.tableand I'd appreciate any feedback!