search

tidyverse / hms

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

as.hms.POSIXct should respect time zone #28

Closed hadley closed 7 years ago

hadley commented 7 years ago

Or there should be another function that allows you to extract the clock time from a POSIXct object (because it's often useful to separate a date time into a date and a time)

hadley commented 7 years ago

It might better to have a separate function for this which takes an explicit tz — this is useful when you have a vector of datetimes in UTC and a vector of local time zones and want to get the local time of day.

krlmlr commented 7 years ago

I feel this is out of scope for this package. Isn't there an equivalent lubridate function?

hadley commented 7 years ago

It feels like this is going to be a common way to create hms objects - it's often useful to partition a date-time into a date and a time. That feels very much in scope for hms (and I don't think there's an existing lubridate function for this)

krlmlr commented 7 years ago

I have found x - lubridate::floor_date(x, "days"), and lubridate::with_tz(). Happy to implement lubridate::time_of_day() (which should return a Duration) and as.hms() methods in lubridate.

You described a common way to create time-of-day objects, but if you're dealing with timestamps, using lubridate seems a good idea anyway. I'd like to restrict the scope of hms, so unless there's a compelling reason to extend this package to handle dates, I'd prefer implementing ths in lubridate. Are we going to integrate lubridate in the tidyverse, or will there be a modern successor?

krlmlr commented 7 years ago

as.hms.POSIXt() now has a tz argument, default UTC.

hadley commented 7 years ago

It should default to the tz in x?

krlmlr commented 7 years ago

Perhaps yes, but that would break compatibility. If this is important, I'd add a pkgconfig switch.

hadley commented 7 years ago

I think it's probably fine to break compatibility given the number of uses, and the likelihood that anyone actually wants the local time in UTC.

krlmlr commented 7 years ago

Agreed. The new default is now the local tz. I'd like to promote "pkgconfig" usage even if few will use it here.

github-actions[bot] commented 3 years ago

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.