nhs-r-community / NHSRplotthedots

An SPC package to support NHSE/I 'Making Data Count' programme
https://nhs-r-community.github.io/NHSRplotthedots/
Other
47 stars 21 forks source link

Error in to_datetime() - no applicable method for 'to_datetime' applied to an object of class "character" #185

Open francisbarton opened 1 year ago

francisbarton commented 1 year ago

There's a strange error in one of the tests.

Error in UseMethod("to_datetime") : 
  no applicable method for 'to_datetime' applied to an object of class "character"

If you look at /tests/testthat/test-ptd_create_ggplot.R#L109

test_that("it sets the x_axis_breaks correctly", {
  m <- mock()
  stub(ptd_create_ggplot, "scale_x_datetime", m)

  set.seed(123)
  d <- data.frame(x = to_datetime("2020-01-01") + 1:20, y = rnorm(20))

  ...

The tests below this one use as.Date(), which works fine. Do we want a date, or a datetime? I can't see why the to_datetime.character Method wouldn't work on "2020-01-01". It should just call as.POSIXct().

Could we just use lubridate::as_datetime() throughout the package, instead of the internal to_datetime()? I think that function will handle the different cases that to_datetime() tries to handle - but perhaps there is a nuance here about exactly what the output needs to be. (I haven't gone into learning the differences between POSIXt and POSIXct!)

[lubridate is now in the core tidyverse so it doesn't feel too unreasonable to add it as a dependency, given we already have dplyr and stringr and ggplot2 of course.]

Does this particular test need to_datetime, or would as.Date() or lubridate::as_date/as_datetime work just as well? For now I am just going to edit the test to use as.Date() instead.

Here's a very simple reprex:

NHSRplotthedots:::to_datetime("2020-01-01")
#> Error in UseMethod("to_datetime"): no applicable method for 'to_datetime' applied to an object of class "character"
as.Date("2020-01-01")
#> [1] "2020-01-01"
as.POSIXct("2020-01-01")
#> [1] "2020-01-01 GMT"
lubridate::as_datetime("2020-01-01")
#> [1] "2020-01-01 UTC"

@tomjemmett

Related: #169 ?

tomjemmett commented 12 months ago

will have a look at this.

including {lubridate} would be a last resort - it's only needed in one place, and including an extra dependency has issues (it's advised you stay below something like 20 dependencies when submitting to CRAN)