Skip to content

Conversation

Nosferican
Copy link
Contributor

Extending functions rather than undocumented new ones.

This PR modifies the undocumented/not exported zoned flavored conversions to methods expanding the Dates API to just work with the ZonedDateTime.
@Nosferican
Copy link
Contributor Author

Nosferican commented Oct 9, 2019

Issue with building Julia 1.3 with Win unrelated (JuliaLang/julia#33470).

@Nosferican
Copy link
Contributor Author

Tests should be passing now with the release of Julia 1.3-RC4. It needs a re-trigger.

@Nosferican
Copy link
Contributor Author

Bump.

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

function zdt2julian(zdt::ZonedDateTime)
datetime2julian(utc(zdt))
end
datetime2julian(zdt::ZonedDateTime) = datetime2julian(utc(zdt))
Copy link
Member

Choose a reason for hiding this comment

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

My preference would be to extend via:

Suggested change
datetime2julian(zdt::ZonedDateTime) = datetime2julian(utc(zdt))
Dates.datetime2julian(zdt::ZonedDateTime) = datetime2julian(utc(zdt))

The rest of this package extends this way so I'd like to keep it consistent


function zdt2julian(::Type{T}, zdt::ZonedDateTime) where T<:Integer
floor(T, datetime2julian(utc(zdt)))
end
Copy link
Member

Choose a reason for hiding this comment

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

Deprecations need to be added for all of the removed functions

function unix2zdt(seconds::Real)
ZonedDateTime(unix2datetime(seconds), utc_tz, from_utc=true)
end
unix2datetime(::Type{<:ZonedDateTime}, seconds::Real) = ZonedDateTime(unix2datetime(seconds), utc_tz, from_utc=true)
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to keep the line length to 92

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