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

date_format does not work correctly on hms objects #88

Closed
trittweiler opened this Issue May 9, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@trittweiler

trittweiler commented May 9, 2017

> library(hms)
> library(scales)
> t <- as.hms(Sys.time())
> date_format("%H:%M", tz="GMT")(t)
[1] "12:30:20.18367"
> format(t, "%H:%M", tz="GMT")
[1] "12:30:20.18367"
> strftime(t, "%H:%M", tz="GMT")
[1] "12:30"

As the function is called "date_format" behaving like "format" is perhaps the right behaviour.

Is this perhaps rather an issue with the hms library which should implement some kind of format method?

@karawoo

This comment has been minimized.

Collaborator

karawoo commented Jun 14, 2017

If I understand correctly the issue here is that date_format behaves differently for POSIXct and hms objects, and for hms objects it shows seconds even when the format requested is only "%H:%M". A more illustrative example is below:

library(hms)
library(scales)

t <- Sys.time()

date_format("%H:%M", tz = "GMT")(t)
#> [1] "17:22"
date_format("%H:%M", tz = "GMT")(as.hms(t))
#> [1] "17:22:08.155499"

format(t, "%H:%M", tz = "GMT")
#> [1] "17:22"
format(as.hms(t), "%H:%M", tz = "GMT")
#> [1] "17:22:08.155499"

date_format uses format under the hood, so that is why they behave the same. @hadley do you think this behavior should change? Or is this issue more appropriate for the hms package?

@hadley

This comment has been minimized.

Member

hadley commented Jun 14, 2017

We should probably add support for hms.

@dpseidel

This comment has been minimized.

Collaborator

dpseidel commented Jun 7, 2018

So this is caused because the format.hms method converts hms objects to character, ultimately calling format.Default and ignoring the format argument to date_format (and the underlying methods format.POSIXlt and format.POSIXct)

Assuming we want to fix this in scales without adjusting the format.hms method itself, the simplest way is to call format.Date directly, although I think this is generally considered bad practice. format.Date and strftime (just a wrapper for format.POSIXlt) seem to differ in how the timezone is treated but (with proper consideration of the timezone) either will output the expected result (as shown using strftime in the OP).

This fix of course stops making sense when a user provides an hms object and asks for a format containing dates not just hours minutes and seconds (as is currently the default for date_format). Perhaps it's worth providing hms support in a separate function time_format? Or is it more desirable to generalize date_format with specific handling of hms objects so they print using a format method for date/time objects?

@hadley

This comment has been minimized.

Member

hadley commented Jun 14, 2018

I'd rewrite date_format() along these lines:

date_format <- function(format = "%Y-%m-%d", tz = 'UTC') {
  function(x) {
    if (inherits(x, "POSIXt")) {
      format(x, format, tz = tz)  
    } else if (inherits(x, "Date")) {
      
    } else if (inherits(x, "difftime")) {
      
    } else {
      stop("Unsupported type (needs better error message)")
    }
      
  }
}

Alternatively, it might be better to add a custom time format type — it's a bit odd to use date_format() with a time.

dpseidel added a commit to dpseidel/scales that referenced this issue Jun 27, 2018

dpseidel added a commit to dpseidel/scales that referenced this issue Jun 28, 2018

dpseidel added a commit to dpseidel/scales that referenced this issue Jun 28, 2018

@dpseidel dpseidel closed this in ad6f084 Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment