NHSRplotthedots icon indicating copy to clipboard operation
NHSRplotthedots copied to clipboard

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

Open francisbarton opened this issue 1 year ago • 1 comments

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 ?

francisbarton avatar Sep 08 '23 08:09 francisbarton

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)

tomjemmett avatar Sep 19 '23 12:09 tomjemmett