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

Native Elixir DateTime handling #84

Closed
wants to merge 7 commits into from
Closed

Conversation

gtebbutt
Copy link

The immediate fix for #64 seems to be feeding the parameters through Moebius.Transformer.from_time_struct, ensuring that any dates/times are parsed out.

Going a little further, since the latest versions of timex and postgrex use the native Elixir DateTime, it seemed sensible to make use of them. This raises two questions:

  • How to handle timezones when they aren't provided. Currently they're assumed to be UTC, but I'm not certain what the cleanest approach is...
  • Whether to return a DateTime or a string for timestamp columns. Currently returning the former, which gives flexibility and matches the latest postgrex API, but does mean it would be a breaking change for moebius.

Any thoughts, @robconery?

@gtebbutt
Copy link
Author

I'm not entirely sure why this is failing - all tests pass locally. If anyone feels like double checking on their machine I'd appreciate it!

@robconery
Copy link
Owner

Oh my god you're my new favorite person. WOW dates have been so, so hard to deal with!

The only concern I would have is breaking old implementation of Elixir. Currently Moebius is set to require 1.1 or greater; but the Date stuff is in 1.2 I believe?

@gtebbutt
Copy link
Author

High praise indeed! I actually picked up Elixir using Red:4, so credit right back to you there.

Just checked the changelog and the new types are only in 1.3, so it would limit compatibility to the newest Elixir version only. Coming from the Clojure world it's pretty standard to push users onto the latest language release, but I'm not sure what the general etiquette is in Elixir-land?

@robconery
Copy link
Owner

Yeah... I can't do this until we rev to 3.0. I don't have a timeline for that, but if people updated this would break everyone's app. Ugh.

@gtebbutt
Copy link
Author

No worries, the code's there whenever it's needed. In the meantime I'll split off the small standalone fix for issue #64 as a separate PR.

@fishcakez
Copy link

A quick note, master (and so the next version) of postgrex will require Elixir ~> 1.3 in order to default to new calendar types and there is a new extensions behaviour for much faster decoding. I would expect a new release of postgrex and elixir this month.

@robconery
Copy link
Owner

Yeah, I'm being squeezed to push to v3 and support 2 branches moving forward. Timex requires 1.3 too. This is a sucky position to be in.

@CrowdHailer
Copy link
Contributor

Does Moebius support any way of casting the values in date fields back to a Timex.Date struct, as it allows them to be passed in? Or is it a case of making your own mapper on top?

@robconery
Copy link
Owner

That part is up to you I believe.

@robconery
Copy link
Owner

Closing this as v3 will move entirely to whatever Postgrex returns, which at the moment is a DateTime struct. You can use this with Timex however you like - which is great. V3 should be out shortly.

@robconery robconery closed this Feb 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants