Skip to content

Fix for issue with sqlite3 datetime parsing#56

Open
fernandosanchezjr wants to merge 1 commit intocoocood:masterfrom
fernandosanchezjr:master
Open

Fix for issue with sqlite3 datetime parsing#56
fernandosanchezjr wants to merge 1 commit intocoocood:masterfrom
fernandosanchezjr:master

Conversation

@fernandosanchezjr
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Author

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

@coocood
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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