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

force_tz() should convert dates to date-times #675

Closed
ClaytonJY opened this issue May 17, 2018 · 5 comments
Closed

force_tz() should convert dates to date-times #675

ClaytonJY opened this issue May 17, 2018 · 5 comments
Labels

Comments

@ClaytonJY
Copy link

@ClaytonJY ClaytonJY commented May 17, 2018

Perhaps this is intentional, but it strikes me as unexpected:

library(lubridate)
#> 
#> Attaching package: 'lubridate'
#> The following object is masked from 'package:base':
#> 
#>     date

tz(today())
#> [1] "UTC"

tz(force_tz(today(), "America/New_York"))
#> [1] "UTC"

tz(with_tz(today(), "America/New_York"))
#> [1] "America/New_York"

Latest Github version, 1.7.4.

In general, timezones for pure Dates seem to be discouraged in that virtually no lubridate functions will ever return a non-UTC date (e.g. today, as_date, date, etc.), but I can't find anything to this effect in the docs.

@vspinu
Copy link
Member

@vspinu vspinu commented Jun 1, 2018

I think the last two should return POSIX instead of date. I would qualify this as a bug.

Do you also find the first example unexpected? This is a new addition in the last minor version to avoid dealing explicitly with the NULL tzs, but I am not entirely convinced it was such a good idea.

@ClaytonJY
Copy link
Author

@ClaytonJY ClaytonJY commented Jun 2, 2018

I strongly recommend against up-converting dates to datetime in general, and I think doing so implicitly would be asking for trouble. The typical approach there is to assume the time is all-zeroes, but then with_tz can easily change the date which I suspect is rarely the desired outcome.

I hadn't thought of it when I created this issue, but on reflection, yes I think the first result is unexpected, given that I have a non-UTC locale set.

Would it make sense to drop timezones from time-free Dates altogether? e.g. tz would error or return NULL, with_tz/force_tz could warn or error, etc.

Potential problems might include edge cases like Samoa's missing day (https://zachholman.com/talk/utc-is-enough-for-everyone-right).

@vspinu
Copy link
Member

@vspinu vspinu commented Jun 2, 2018

You ask for an operation which makes sense on date-times, that implise that you are interpreting the date as an instant. Otherwise why would you do force_tz on a date?

Would it make sense to drop timezones from time-free Dates altogether?

Yes. I will revert it to NULL.

@ClaytonJY
Copy link
Author

@ClaytonJY ClaytonJY commented Jun 2, 2018

I agree force_tz doesn't make sense; I only tried it because I didn't get the expected behavior from with_tz. For consistency across a mix of dates and datetimes, my goal was to set the correct timezone, and tz(x) <- "America/New_York" leads to awkward code. After this discussion I don't even want to do that; I think we're in agreence that dates should not have timezones in any manner.

I'm happy to help with code, though I won't have time until at least tomorrow. Sounds like you want tz to return NULL; what about force_tz/with_tz? Erroring would make it clear lubridate doesn't support timezones on dates, but might also break users code. Could throw a warning in one release and an error in a subsequent release if breakage is a concern, but it's possible nobody else is trying to set timezones on their dates like I did.

@hadley
Copy link
Member

@hadley hadley commented Nov 19, 2019

It seems reasonable to me to return POSIXct when you explicit request a timezone be added to a Date. Adding a tzone attribute to a Date object is definitely wrong:

library(lubridate, warn.conflicts = FALSE)

tz(force_tz(today(), "America/New_York"))
#> [1] "UTC"
class(force_tz(today(), "America/New_York"))
#> [1] "Date"

tz(with_tz(today(), "America/New_York"))
#> [1] "America/New_York"
class(with_tz(today(), "America/New_York"))
#> [1] "Date"

tz(`tz<-`(today(), "America/New_York"))
#> [1] "UTC"
class(`tz<-`(today(), "America/New_York"))
#> [1] "Date"

Created on 2019-11-20 by the reprex package (v0.3.0)

(I agree that lubridate seems to upclass dates to date-times too readily; but that's a separate issue)

@hadley hadley changed the title force_tz doesn't work on pure Dates force_tz() should convert dates to date-times Nov 19, 2019
@hadley hadley added the bug label Nov 19, 2019
@hadley hadley added the wip label Nov 20, 2019
@vspinu vspinu closed this in #825 Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.