Skip to content
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

as.hms.POSIXct should respect time zone #28

Closed
hadley opened this issue Apr 23, 2017 · 10 comments
Closed

as.hms.POSIXct should respect time zone #28

hadley opened this issue Apr 23, 2017 · 10 comments

Comments

@hadley
Copy link
Member

hadley commented Apr 23, 2017

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
Copy link
Member Author

hadley commented Apr 23, 2017

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
Copy link
Member

krlmlr commented Apr 25, 2017

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

@hadley
Copy link
Member Author

hadley commented Apr 25, 2017

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
Copy link
Member

krlmlr commented Apr 26, 2017

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
Copy link
Member

krlmlr commented May 1, 2017

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

@hadley
Copy link
Member Author

hadley commented May 1, 2017

It should default to the tz in x?

@krlmlr
Copy link
Member

krlmlr commented May 2, 2017

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

@hadley
Copy link
Member Author

hadley commented May 2, 2017

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
Copy link
Member

krlmlr commented May 2, 2017

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

krlmlr added a commit that referenced this issue Jul 25, 2017
- Fix and enhance examples in `?hms`.
- `hms()` creates a zero-length object of class `hms` that prints as `"hms()"`.
- `hms(integer())` and `as.hms(integer())` both work and are identical to `hms()`.
- `as.hms.POSIXt()` now defaults to the current time zone, the previous default was `"UTC"` and can be restored by calling `pkgconfig::set_config("hms::default_tz", "UTC")`.
- `as.hms.POSIXt()` gains `tz` argument, default `"UTC"` (#28).
krlmlr added a commit that referenced this issue Nov 23, 2017
Breaking changes
----------------

- `as.hms.POSIXt()` now defaults to the current time zone, the previous default was `"UTC"` and can be restored by calling `pkgconfig::set_config("hms::default_tz", "UTC")`.

New features
------------

- Pillar support, will display `hms` columns in tibbles in color on terminals
  that support it (#43).
- New `round_hms()` and `trunc_hms()` for rounding or truncating to a given multiple of seconds (#31).
- New `parse_hms()` and `parse_hm()` to parse strings in "HH:MM:SS" and "HH:MM" formats (#30).
- `as.hms.POSIXt()` gains `tz` argument, default `"UTC"` (#28).
- `as.hms.character()` and `parse_hms()` accept fractional seconds (#33).

Bug fixes
---------

- `hms()` now works correctly if all four components (days, hours, minutes, seconds) are passed (#49).
- `hms()` creates a zero-length object of class `hms` that prints as `"hms()"`.
- `hms(integer())` and `as.hms(integer())` both work and are identical to `hms()`.
- Values with durations of over 10000 hours are now printed correctly (#48).
- `c()` now returns a hms (#41, @qgeissmann).

Documentation and error messages
--------------------------------

- Fix and enhance examples in `?hms`.
- Documentation is in Markdown format now.
- Improved error message if calling `hms()` with a character argument (#29).
@github-actions
Copy link

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.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants