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

To datetime with timezone #226

Merged
merged 4 commits into from Oct 11, 2016
Merged

Conversation

jadlr
Copy link
Contributor

@jadlr jadlr commented Oct 11, 2016

This changes the behavior of to_datetime when called with a DateTime struct.

I'm writing an application that needs to convert datetimes to other timezones and I didn't know about Timex.Timezone.convert/2

I was surprised by the current behavior:

iex(1)> Timex.to_datetime(Timex.now, "Europe/Berlin") 
#<DateTime(2016-10-11T16:05:08.439973Z Etc/UTC)>

I was even more surprised by:

iex(2)> Timex.to_datetime(Timex.now, "Hello")        
#<DateTime(2016-10-11T16:05:45.608048Z Etc/UTC)>

I think the more logical thing to do is to convert the date if the timezone differs from the one in the DateTime struct.

What do you think? Is there something I am overlooking?

@coveralls
Copy link

coveralls commented Oct 11, 2016

Coverage Status

Coverage increased (+0.01%) to 64.812% when pulling 6ed44ee on trsc:to_datetime_with_timezone into f0f1753 on bitwalker:master.

@@ -39,7 +39,8 @@ defimpl Timex.Protocol, for: DateTime do
def to_date(date), do: DateTime.to_date(date)

@spec to_datetime(DateTime.t, timezone :: Types.valid_timezone) :: DateTime.t | {:error, term}
def to_datetime(%DateTime{} = d, _timezone), do: d
def to_datetime(%DateTime{time_zone: time_zone} = d, timezone) when time_zone == timezone, do: d
Copy link
Owner

Choose a reason for hiding this comment

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

You can just do to_datetime(%DateTime{time_zone: timezone} = d, timezone), do: d which will only match if both the provided timezone and the one on the DateTime are the same.

@bitwalker
Copy link
Owner

You're definitely correct, your changes look good except for the one tweak I suggested. I'll get this merged once that's updated. Thanks for the PR!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 64.812% when pulling 6b0fe32 on trsc:to_datetime_with_timezone into f0f1753 on bitwalker:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 64.812% when pulling 6b0fe32 on trsc:to_datetime_with_timezone into f0f1753 on bitwalker:master.

@jadlr
Copy link
Contributor Author

jadlr commented Oct 11, 2016

@bitwalker thanks for the hint! I didn't know that.

I just found out that Timex.Timezone.convert can return an AmbiguousDateTime. Could that be a problem? Should I add AmbiguousDateTime to the @spec of to_datetime?

@bitwalker
Copy link
Owner

Yeah could you add it to the @spec, it should be there. AmbiguousDateTime is intended to be resolved by the caller, but it should only occur with DateTime, no other types, so we need to pass it back to the caller for any conversions like this which may result in an AmbiguousDateTime.

@coveralls
Copy link

coveralls commented Oct 11, 2016

Coverage Status

Coverage increased (+0.01%) to 64.812% when pulling da9513f on trsc:to_datetime_with_timezone into f0f1753 on bitwalker:master.

@jadlr
Copy link
Contributor Author

jadlr commented Oct 11, 2016

@bitwalker OK, done

@bitwalker bitwalker merged commit 2a1d96d into bitwalker:master Oct 11, 2016
@bitwalker
Copy link
Owner

Thanks again!

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.

None yet

3 participants