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

Add generic operations for hms #120

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AshesITR
Copy link

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
Copy link
Author

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

@krlmlr
Copy link
Member

krlmlr commented Aug 22, 2023

Thanks. The existing tests currently fail?

@AshesITR
Copy link
Author

Yes, the arith-tests regarding POSIX times.

@AshesITR AshesITR marked this pull request as ready for review August 22, 2023 05:39
@AshesITR
Copy link
Author

NB We produce this message:

Registered S3 methods overwritten by 'hms':
  method   from
  +.Date   base
  +.POSIXt base
  -.Date   base
  -.POSIXt base

@AshesITR
Copy link
Author

AshesITR commented Aug 22, 2023

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
Copy link
Member

krlmlr commented Aug 23, 2023

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
Copy link
Author

AshesITR commented Aug 24, 2023

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
Copy link
Author

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

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.

/ et al. drop class "hms" hms class dropped when doing arithmetic on hms objects
2 participants