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

Fix for issue with sqlite3 datetime parsing #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix for issue with sqlite3 datetime parsing #56

wants to merge 1 commit into from

Conversation

fernandosanchezjr
Copy link

Fix for error parsing time "2015-12-02 19:15:53.964608741-08:00": extra text: -08:00 when loading time.Time fields from sqlite3 TEXT fields

Fix for error `parsing time "2015-12-02 19:15:53.964608741-08:00": extra text: -08:00` when loading time.Time fields from sqlite3 TEXT fields
@coocood
Copy link
Owner

coocood commented Dec 3, 2015

Can you write a test for this issue. Thank you.
And have you tested that this change is compatiable with old data?

@fernandosanchezjr
Copy link
Author

Oy. Do you have a sample of "old data" that I could use?

@coocood
Copy link
Owner

coocood commented Dec 3, 2015

I mean the data created before this change, before this change, tables was created with 'TEXT" column type for time.Time go type.

And can you explain why this change fixes this issue?

@fernandosanchezjr
Copy link
Author

The field is still created as a TEXT field. This issue, as I traced it, seems to have more to do with your use of github.com/mattn/go-sqlite3. Making the sqlType return "datetime" seems to be able to handle one of the proper time.Time formats as defined in https://github.com/mattn/go-sqlite3/blob/master/sqlite3.go line 101 onwards.

Seeing as how a list of formats is outlined in go-sqlite3/sqlite3.go, a set of tests that verifies each format as stored in a TEXT field (as done by qbs as is when it is allowed to create tables with CreateTableIfNotExists) could be a sensical deliverable from me.

Any test cases you already have in mind or vetted by you (I assume you have seen a thing or two since you are the guy building an ORM) in the form of some sort of db dump would also work wonders.

What are your thoughts?

@fernandosanchezjr
Copy link
Author

Whoops. Clicked the wrong thing. Tomorrow I will take the time to trace through and come up with a complete explanation of why the fix works for my needs, as well.

@coocood
Copy link
Owner

coocood commented Dec 3, 2015

OK, now I know why this change fixes that issue. Please add a test cover this issue, then I will merge it.
Thank you.

@fernandosanchezjr
Copy link
Author

No problem. Will do. Thanks for all the effort on qbs.

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.

2 participants