search

tidyverse / hms

A simple class for storing time-of-day values
https://hms.tidyverse.org/
Other
138 stars 25 forks source link

Add generic operations for `hms` #120

Open AshesITR opened 1 year ago

AshesITR commented 1 year ago

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 regarding difftime has no manual disambiguation, firing the warning.

Fixes #119 Fixes #18

AshesITR commented 1 year ago

NTS: Should also check this plays nicely with {lubridate} et al.

krlmlr commented 1 year ago

Thanks. The existing tests currently fail?

AshesITR commented 1 year ago

Yes, the arith-tests regarding POSIX times.

AshesITR commented 1 year ago

NB We produce this message:

Registered S3 methods overwritten by 'hms':
  method   from
  +.Date   base
  +.POSIXt base
  -.Date   base
  -.POSIXt base
AshesITR commented 1 year ago

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? Also, how to proceed?

Create backward-compatible behavior in the old versions (likely by branching in Ops.hms based on getRVersion() < "4.1")? Looking at the failures, this seems like the right thing to do, because test-arith.R also produces failures on those versions.

krlmlr commented 1 year ago

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.

AshesITR commented 1 year ago

Wow, what a weird failure. Not exporting the methods is ignored by R <= 4.0, causing test-arith failures and an incompatibility warning 😤

Incompatible methods ("+.Date", "Ops.hms") for "+"

The incompatibility warning will not be fixable in R < 4.1 because this fix is needed in eval.c:

https://github.com/wch/r-source/blob/61468d9909b8ab59a25172e584e4400427b40a27/src/main/eval.c#L4114-L4140

https://github.com/wch/r-source/blob/97fee142299e5b418cdaf1057eca0e12c250a2b8/src/main/eval.c#L3796-L3804

Seems like I have to add a working implementation for old R versions, too.

AshesITR commented 10 months ago

I'm failing to reproduce the failure on 4.0.4 @ windows.