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

Draft for updating conversions #224

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Draft for updating conversions #224

wants to merge 2 commits into from

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