Date <-> ZonedDateTime #231

oxinabox · 2019-11-21T16:09:18Z

I got really tired of writing Date(DateTime(zdt)) and ZonedDateTime(DateTime(date))

julia> zdt = ZonedDateTime(Date(2011, 6, 1), tz"America/Winnipeg")
2011-06-01T00:00:00-05:00

julia> Date(zdt)
2011-06-01

julia> DateTime(Date(2011, 6, 1))
2011-06-01T00:00:00

omus · 2019-11-21T16:16:08Z

src/conversions.jl

+
+Returns an equivalent `Date` without any `TimeZone`, or time information.
+"""
+Dates.Date(zdt::ZonedDateTime) = Date(DateTime(zdt))


I've been intending on dropping DateTime(::ZonedDateTime) in favor of localtime(::Type{DateTime}, ::ZonedDateTime) or something similar. This is definitely a lossy operation and it's important to communicate the implicit time zone.

I would really dislike that.
Often I just have a collection of DateTime,
one per day.
and I want to plot them (cf #184),
or I want to display them in a table,
and I just want to get rid of extra information.
I don't actually care about the time or even the exact date often.

I think it pretty clear to the user that it is losing information.

Looking at that issue again Date(zdt, UTC) / Date(zdt, Local) isn't bad

I am happy to see this change when we resolve #125 for DateTimes.
Til then this match the API we have

If you don't want to take on the change yourself I can make the remainder of the changes. I don't think I would merge this as is.

I do want to merge as is, because the status quo of matching how DateTime works is known.
All other things are not known and we might want to do a little design and thinking.

OTOH:

Date(zdt, Local) and Date(zdt, UTC) // DateTime(zdt, Local) and DateTime(zdt, UTC) do sound right to me though.

A bunch of places that do DateTime(ztd) will need to be changed due to deprecation. and any places that do Date(DateTime(ztd)) will be a subset of those. So doing both at once would mean we can go staight to Date(ztd, Local).

I think I am convinced.

I think I am convinced

Convinced enough to do the work? ;). As I said I can do the leg work if you prefer.

I'm not sure how deep I would have to go,
so if you know exactly what you have to change be my guest.

Otherwise this might sit til after neurips

Waiting til after NeurIPS might be fine though.
We've lived this long

test/conversions.jl

codecov-io · 2019-11-21T17:09:45Z

Codecov Report

Merging #231 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   93.97%   93.99%   +0.02%     
==========================================
  Files          29       29              
  Lines        1112     1116       +4     
==========================================
+ Hits         1045     1049       +4     
  Misses         67       67

Impacted Files	Coverage Δ
src/TimeZones.jl	`100% <ø> (ø)`	⬆️
src/discovery.jl	`87.5% <ø> (ø)`	⬆️
src/types/zoneddatetime.jl	`93.44% <100%> (+0.1%)`	⬆️
src/conversions.jl	`100% <100%> (ø)`	⬆️
src/arithmetic.jl	`82.35% <100%> (ø)`	⬆️
src/rounding.jl	`100% <100%> (ø)`	⬆️
src/io.jl	`96.29% <100%> (ø)`	⬆️
src/accessors.jl	`80% <100%> (-5.72%)`	⬇️
src/adjusters.jl	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed6288e...56da670. Read the comment docs.

omus · 2019-12-03T20:23:16Z

Alright, I've made the updates and bumped the version to 1.0.0 (it's time)

src/TimeZones.jl

src/adjusters.jl

src/deprecated.jl

src/conversions.jl

oxinabox · 2019-12-03T21:29:04Z

src/conversions.jl


 function zdt2unix(::Type{T}, zdt::ZonedDateTime) where T<:Integer
-    floor(T, datetime2unix(utc(zdt)))
+    floor(T, datetime2unix(DateTime(zdt, UTC)))


Suggested change

floor(T, datetime2unix(DateTime(zdt, UTC)))

return floor(T, datetime2unix(DateTime(zdt, UTC)))

oxinabox · 2019-12-03T21:29:17Z

src/conversions.jl


 function zdt2unix(zdt::ZonedDateTime)
-    datetime2unix(utc(zdt))
+    datetime2unix(DateTime(zdt, UTC))


Suggested change

datetime2unix(DateTime(zdt, UTC))

return datetime2unix(DateTime(zdt, UTC))

oxinabox · 2019-12-03T21:30:10Z

src/conversions.jl

 end

 function julian2zdt(jd::Real)
    ZonedDateTime(julian2datetime(jd), utc_tz, from_utc=true)


Suggested change

ZonedDateTime(julian2datetime(jd), utc_tz, from_utc=true)

return ZonedDateTime(julian2datetime(jd), utc_tz, from_utc=true)

oxinabox · 2019-12-03T21:30:23Z

src/conversions.jl


 function zdt2julian(::Type{T}, zdt::ZonedDateTime) where T<:Real
-    convert(T, datetime2julian(utc(zdt)))
+    convert(T, datetime2julian(DateTime(zdt, UTC)))


Suggested change

convert(T, datetime2julian(DateTime(zdt, UTC)))

return convert(T, datetime2julian(DateTime(zdt, UTC)))

oxinabox · 2019-12-03T21:30:29Z

src/conversions.jl


 function zdt2julian(::Type{T}, zdt::ZonedDateTime) where T<:Integer
-    floor(T, datetime2julian(utc(zdt)))
+    floor(T, datetime2julian(DateTime(zdt, UTC)))


Suggested change

floor(T, datetime2julian(DateTime(zdt, UTC)))

return floor(T, datetime2julian(DateTime(zdt, UTC)))

oxinabox · 2019-12-03T21:30:39Z

src/conversions.jl


 function zdt2julian(zdt::ZonedDateTime)
-    datetime2julian(utc(zdt))
+    datetime2julian(DateTime(zdt, UTC))


Suggested change

datetime2julian(DateTime(zdt, UTC))

return datetime2julian(DateTime(zdt, UTC))

omus · 2019-12-13T17:12:14Z

I squashed some commits in preparation for merging. A couple of things to note:

Adding return statements will not be done in this PR as I think that is a separate conversation
We're bumping to version 1.0 here as there haven't been any major API changes for a while and the ones I have planned can always go into 2.0

oxinabox · 2019-12-13T18:15:47Z

src/conversions.jl

+"""
+    Time(zdt::ZonedDateTime, ::Union{Type{Local}, Type{UTC}}) -> Time
+
+Create an implicit local/UTC `Time` from the given `ZonedDateTime`.


Couild we add a bit more text?

Suggested change

Create an implicit local/UTC `Time` from the given `ZonedDateTime`.

Create an implicit local/UTC `Time` from the given `ZonedDateTime`.

- `Local` means a time local to the timezone in the `ZonedDateTime`, i.e. just discarding the timezone information. (**Not** local to the system time of the user invoking the code).

- `UTC` means to shift to the `UTC` timezone and then discard all timezone information.

Maybe also this particular text should be put in to a string constant and interpolatred in at a few places?
Like in Date and DateTime as well?

Probably could be written better

I rewrote the docstrings to be more sensible

oxinabox

Other than a bit of documentation this LGTM.

omus · 2020-01-23T15:34:55Z

Quick note: I'm still planning on doing the 1.0 version change as part of this but since this adds a deprecation I'll be doing a 0.11 release first so I can drop the deprecation as part of 1.0

mattBrzezinski · 2020-01-23T15:56:52Z

src/TimeZones.jl


 export TimeZone, @tz_str, istimezone, FixedTimeZone, VariableTimeZone, ZonedDateTime,
-    DateTime, TimeError, AmbiguousTimeError, NonExistentTimeError, UnhandledTimeError,
+    DateTime, Date, Time, UTC, Local, TimeError, AmbiguousTimeError, NonExistentTimeError,


Could these be broken down into logical groupings (macros, errors, etc.) or alphabetically?

I can do some organization. What do you think of grouping by the file in which these are defined (and having a comment indicating the file)?

This might be too granular, but:

export @tz_str export AmbiguousTimeError, NonExistentTimeError, TimeError export Date, DateTime, Local, Time, TimeZone, UTC, VariableTimeZone, ZonedDateTime export isdatetime

I think it's less about what the groupings are but more that there is some order to them.

I'll note that there is an ordering to this but it's not as obvious as I'd like. A couple of key things I would like to communicate as part of the exports:

Quite a few of these are re-exports from Dates which are just here in case someone only does using TimeZones

There are some types that are closely related like UTC and Local which I think should be sequential

I'll post an revised version in a sec

What I'm currently thinking:

export TimeZone, FixedTimeZone, VariableTimeZone, @tz_str, istimezone, localzone UTC, Local, ZonedDateTime, timezone, # Exceptions TimeError, AmbiguousTimeError, NonExistentTimeError, UnhandledTimeError, # Discovery tools timezone_names, all_timezones, timezones_from_abbr, timezone_abbrs, next_transition_instant, show_next_transition, # Extended functionality from Dates. Note: Some of these exports have already been done # but are listed here for completeness. TimeZone, UTC, DateTime, Date, Time, now, today, year, month, week, day, dayofmonth, hour, minute, second, millisecond yearmonthday, yearmonth, monthday, firstdayofyear, firstdayofquarter, firstdayofmonth, firstdayofweek, lastdayofyear, lastdayofquarter, lastdayofmonth, lastdayofweek

I'm unsure on what is better at this point and possibly this kind of logical grouping should just be in the documentation? In any case I think this could be made into an issue and not hold up this PR.

Agreed, this isn't a dealbreaker for the PR going through and can be discussed afterwards.

Side-note; does exporting functionality multiple times have any consequences to it?

does exporting functionality multiple times have any consequences to it?

No it shouldn't

src/discovery.jl

omus reviewed Nov 21, 2019

View reviewed changes

fchorney reviewed Nov 21, 2019

View reviewed changes

test/conversions.jl Show resolved Hide resolved

omus reviewed Nov 21, 2019

View reviewed changes

test/conversions.jl Outdated Show resolved Hide resolved

test/conversions.jl Outdated Show resolved Hide resolved