tidyverse / hms

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

`/` et al. drop class "hms" #119

Open AshesITR opened 1 year ago

AshesITR commented 1 year ago

Most computations with hms drop the class to difftime causing worse UX.

hms::hms(10.5) / 2.0
#> Time difference of 5.25 secs
`/.hms` <- function(a, b) {
  res <- as.difftime(unclass(a), units = attr(a, "units")) / b
  hms::as_hms(res)
}
hms::hms(10.5) / 2.0
#> 00:00:05.25
sum(hms::hms(c(10L, 10L)))
#> Time difference of 20 secs

Created on 2023-08-18 with reprex v2.0.2

AshesITR commented 1 year ago

Here is a more general fix, covering base operations and summaries (like sum, min, max, range), and mean() I can work on a PR if the change is desired.

Ops.hms <- function(e1, e2) {
  res <- NextMethod(.Generic)
  if (inherits(res, "difftime")) {
    hms::as_hms(res)
  } else {
    res
  }
}

Summary.hms <- function(..., na.rm) {
  res <- NextMethod(.Generic)
  hms::as_hms(res)
}

mean.hms <- function(x, ...) {
  hms::as_hms(NextMethod(.Generic))
}
krlmlr commented 1 year ago

Thanks. Is this related to #18?

AshesITR commented 1 year ago

Trying to understand the neccessary test cases: Relevant classes appear to be all of: Date, POSIXct and POSIXlt (possibly with timezones and times around DST), difftime (base), hms.

Once with hms on the LHS and once with hms on the RHS (with expect_error() where appropriate).

Is that assessment correct?

krlmlr commented 1 year ago

Multiplying with and dividing by numeric or integer? Otherwise, this looks fairly complete.