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

Loosen type signatures to support UTCDateTimes #211

Merged
merged 2 commits into from
Jan 25, 2023
Merged

Conversation

rofinn
Copy link
Member

@rofinn rofinn commented Jan 25, 2023

No description provided.

@rofinn rofinn requested a review from omus as a code owner January 25, 2023 20:23
@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #211 (4fff628) into master (897bd75) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #211   +/-   ##
=======================================
  Coverage   84.31%   84.31%           
=======================================
  Files          12       12           
  Lines         848      848           
=======================================
  Hits          715      715           
  Misses        133      133           
Impacted Files Coverage Δ
src/anchoredinterval.jl 99.12% <100.00%> (ø)
src/interval.jl 96.01% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -386,8 +386,8 @@ canonicalize(target_type::Type{P}, p::P) where P <: Period = p

##### TIME ZONES #####

function TimeZones.astimezone(i::AnchoredInterval{P, ZonedDateTime, L, R}, tz::TimeZone) where {P,L,R}
function TimeZones.astimezone(i::AnchoredInterval{P, T, L, R}, tz::TimeZone) where {P,T,L,R}
return AnchoredInterval{P, ZonedDateTime, L, R}(astimezone(anchor(i), tz))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return AnchoredInterval{P, ZonedDateTime, L, R}(astimezone(anchor(i), tz))
zdt = astimezone(anchor(i), tz)
return AnchoredInterval{P, typeof(zdt), L, R}(zdt)

This could be done in the future. Probably this type of change would have to happen in a bunch of places for it to work and there's no use case right now, though, so we can hold off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an issue for it. Might want to do that at the same time that we try to make TimeZones an optional dependency.

#212

@rofinn rofinn merged commit ba938f6 into master Jan 25, 2023
@rofinn rofinn deleted the rf/utcdatetimes branch January 25, 2023 22:56
Copy link
Collaborator

@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.

Post-review: seems reasonable. As an aside these TZ specific methods have always been a little weird

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.

4 participants