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

Support something useful for timestamps on stable #92

Closed
sgrif opened this Issue Jan 13, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@sgrif
Member

sgrif commented Jan 13, 2016

Rust's Timespec isn't stable. We should optionally support chrono.

@tomhoule

This comment has been minimized.

tomhoule commented Jan 13, 2016

Well, I have been playing a little with diesel and I wrote a rough port of the rust-postgres support for chrono::DateTime using a wrapper struct to implement the Expression, FromSql<Timestamp> and ToSql<Timestamp> traits. The code seems to work (my tests are not very thorough, and I haven’t checked if it works well for building queries yet, just (de)serialization) for the very limited case it covers.

It assumes everything is UTC and does not try to deal with timezones at the moment, but it should be possible at least to the same extent as what exists in rust-postgres.

The rust-postgres implementation of the feature is in this file

If it can be of any help, I can share, expand, complete and polish up the code, write tests and make a PR.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 13, 2016

Sure. If you want to open a PR with what you have, I can do the legwork on finishing it up, too.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 13, 2016

Also I think the only tests that we need for this are an entry in https://github.com/sgrif/diesel/blob/master/diesel_tests/tests/types_roundtrip.rs#L76

It is a shame that we need this in diesel proper -- I really wish that we could make diesel-chrono be a separate crate.

@tomhoule

This comment has been minimized.

tomhoule commented Jan 14, 2016

I worked a little on this, I doubt it is fully functional, but I’ll open a PR right away so you can see if it is worth continuing.

My first implementation was in application code, but of course I had to wrap it in a newtype, and write down the serde traits for serialization, etc. So that was not particularly ergonomic.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 14, 2016

Yeah, that's the main benefit of doing it in crate is not needing to newtype it.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 17, 2016

The only remaining type for this is chrono::NaiveDate. A close to working implementation was done in c0b0fb8 but it was ultimately reverted as I treated 0 as the start of the Julian era, when PG treats it as the start of the PG epoch. Anyone interested should feel free to adapt that code, with tests similar to what have been added to the other date/time types w/ concrete values added for NaiveDate to prove that it's correct.

#106 has been added as a separate tracking issue for figuring out how we should handle timezone aware types.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 18, 2016

Fixed by #114, #101, and #94

@sgrif sgrif closed this Jan 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment