-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add generic operations for hms
#120
base: main
Are you sure you want to change the base?
Conversation
NTS: Should also check this plays nicely with {lubridate} et al. |
Thanks. The existing tests currently fail? |
Yes, the arith-tests regarding POSIX times. |
NB We produce this message:
|
R 3.6 and R 4.0 seem to behave differently. @krlmlr do you have an idea what was changed from 4.0 to 4.1? Create backward-compatible behavior in the old versions (likely by branching in |
I don't mind waiting until R 4.0 is phased out, or at least supporting this feature only for that version. I plan to return to this package some time later in this year and can only offer very superficial advice in the meantime. |
Wow, what a weird failure.
The incompatibility warning will not be fixable in R < 4.1 because this fix is needed in eval.c: Seems like I have to add a working implementation for old R versions, too. |
I'm failing to reproduce the failure on 4.0.4 @ windows. |
Much better now, only R 4.0 fails. Support for that is dropped in April, I think it's good to start reconsidering this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'd really love to see this in hms, would make a good 2.0.0 .
# This logic is hard-coded in R for difftime | ||
# cf. https://github.com/wch/r-source/blob/a46559e8f728317da979be60e401899ae60086b2/src/main/eval.c#L3406-L3419 | ||
if (.Generic == "+" && (inherits(e1, "Date") || inherits(e2, "Date"))) { | ||
return(base::`+.Date`(e1, e2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this wrap as hms?
expect_difftime_equal(2 * hms(1), hms(2)) | ||
expect_difftime_equal(hms(hours = 1) / 2, hms(minutes = 30)) | ||
expect_difftime_equal(-hms(1), hms(-1)) | ||
if (getRversion() < "4.1") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can skip_if(getRversion() < "4.1")
.
@@ -0,0 +1,108 @@ | |||
test_that("generic operations work as intended", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test chunk is too large, can you please split by topic?
I'll see if I can put a little bit more time into this. Spare time for pet projects has taken a cut lately 😅 |
Opening this draft for early feedback.
Missinng POSIXct and POSIXlt currently.
One warning condition (difftime - Date) can't be reproduced exactly using the current implementation.
Let me know if that is a problem, because it seems hard to work around - the compatibility warning will be silenced for good by
-.Date <- Ops.hms
and this constellation is one where the C++ code regardingdifftime
has no manual disambiguation, firing the warning.Fixes #119
Fixes #18