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

Add time_format() function for time formatting #136

Merged
merged 2 commits into from
Jun 28, 2018

Conversation

dpseidel
Copy link
Collaborator

This PR adds a new function, time_format, to format times properly, for both POSIXt and hms objects. A separate function made more sense to me than did adjusting date_format to accommodate times, but if this code feels too redundant, I'm happy to implement changes to date_format instead.

I also wonder if date_format and/or time_format shouldn't be moved to formatter.r with all the other formatters.

Addresses #88


Some notes about handling timezones for hms objects:

For consistency, it seemed important to continue using standard POSIX specification for formatting, but while time zone of hms objects can be specified at conversion (from other datetime classes, defaults to local time) but they not have a tzone attribute themselves and can't/don't respect daylight savings (i.e. a day == 24 hours). Coercing them back to POSIXct is easy but always sets the timezone to "UTC" -- if our function allows the user to change this in higher level format function, times can quickly get misrepresented, especially around daylight savings time. On the other hand, if the user knows the original timezone of the hms object and set it properly during conversion, then being able to play with timezones is maybe a desirable feature.

Ultimately I decided to go with the simplest approach and coerce hms objects to POSIXct and preserve the ability for the user to set the tz in the higher level format function but I'm curious what behavior you think is best.

Some possible alternatives:

  • dropping the tz argument in line 143 OR dropping the tz argument entirely, replacing it with dots and feeding POSIXt objects directly into date_format which could take tz when specified. It seemed a bit odd not to have a timezone argument in a formatting function for times though.
  • coercing to POSIXlt instead which ignores the tz argument in format.
  • adding a warning along the lines of "The timezone of all hms objects is assumed to be UTC. Changing tz may result in incorrect times, please check your data carefully." if the user changes the tz argument and passes an hms object.

Also worth noting, while I hope anyone passing an hms object won't attempt to request a date-time format (especially from the explicitly named time_format function), as it stands, this fix doesn't do anything to prevent or warn users doing this. If they do, the results will be incorrect (i.e. reporting from January 1970).

@dpseidel dpseidel requested a review from hadley as a code owner June 28, 2018 19:09
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

As a follow up, once you've tweaked the error message and merged, can you make another PR that combines the docs for date_format() and time_format()? (And collocate the code)

R/trans-date.r Outdated
format(as.POSIXct(x), format = format, tz = tz)
} else {
stop(
"Objects of class ", paste(class(x), collapse = "/"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a call. = FALSE

And to comply with http://style.tidyverse.org/error-messages.html, I think maybe "not supported by" -> "can't be used with"

@dpseidel dpseidel merged commit ad6f084 into r-lib:master Jun 28, 2018
dpseidel added a commit to dpseidel/scales that referenced this pull request Jun 28, 2018
@dpseidel dpseidel deleted the hms_support branch June 28, 2018 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants