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

Add support for time.Time objects to gorp. #67

Merged
merged 2 commits into from
Sep 15, 2013
Merged

Add support for time.Time objects to gorp. #67

merged 2 commits into from
Sep 15, 2013

Conversation

seehuhn
Copy link
Contributor

@seehuhn seehuhn commented Aug 10, 2013

This commit adds support for storing and retriving time.Time objects,
by passing through such objects to the underlying driver. This
resolves gorp issue #14.

The commit is tested and works with the github.com/mattn/go-sqlite3
driver.

The commit does not currently work with the
github.com/ziutek/mymysql/godrv driver, due to a bug in the mysql
driver. This problem is resolved by mymysql pull request #77. I have
successfully tested that with the fix from pull request #77 applied to
mymysql, and with the current commit applied, gorp can correctly store
and retrieve time.Time objects from mysql databases.

This commit is NOT tested with the github.com/lib/pq driver.

This commit adds support for storing and retriving time.Time objects,
by passing through such objects to the underlying driver.  This
resolves gorp issue #14.

The commit is tested and works with the github.com/mattn/go-sqlite3
driver.

The commit does not currently work with the
github.com/ziutek/mymysql/godrv driver, due to a bug in the mysql
driver.  This problem is resolved by mymysql pull request #77.  I have
successfully tested that with the fix from pull request #77 applied to
mymysql, and with the current commit applied, gorp can correctly store
and retrieve time.Time objects from mysql databases.

This commit is NOT tested with the github.com/lib/pq driver.
@jamesharr
Copy link
Contributor

With timestamp with time zone, the patch works well. Small patch: jamesharr@6724b78be6a1f5fa1d2efdc0ac605b7101d080df

Really, the option should be left up to the user, but Gorp's column mapping doesn't support that yet. Keeping the timezone in postgres works (and without timezone doesn't), so it's the safer option for the time being if you want Time object in Gorp.

Some exploration of the underlying SQL drivers might be warranted as well, since there might be other intricacies that differ between SQL drivers. I know Go-SQL-Driver/MySQL has a location setting for timezones (and a pending pull request to correct conversions). Other drivers may behave differently, especially for MySQL.

@pettyjamesm
Copy link

At this point, gorp is already failing to handle time.Time types. I don't think it's worth holding up this pull request just to figure out the timezones problem. Users already have to work around Time compatibility on their own, they can figure out how to work around timezones if they need to.

@seehuhn
Copy link
Contributor Author

seehuhn commented Sep 8, 2013

I have now updated the pull request to use "timestamp with time zone" as suggested by Joe Shaw. Since I don't have a PostgreSQL installation around, I can't test the change. Could somebody test the new pull request and see whether it all works as expected?

@coopernurse
Copy link
Contributor

I've created a "develop" branch. I'm going to merge into that and test locally. If tests pass I'll push this branch along with a set of other pending pull requests so we can test all of them together before merging to master.

@coopernurse
Copy link
Contributor

postgres and sqlite are passing now. mysql still fails, due to tz issues that @seehuhn notes.

I'm ok with merging this, but I'm going to modify the gorp README to warn users about using time.Time in their mapped structs, particularly on MySQL.

Since the test fails I'm also going to disable the test for now (change it to lowercase so go test doesn't run it). I'll file another issue to remind us to add it back in when ziutek/mymysql#77 lands.

@coopernurse coopernurse merged commit e5a6512 into go-gorp:master Sep 15, 2013
@seehuhn seehuhn deleted the datetimefix branch September 15, 2013 18:36
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