-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
sqlite3: Allow users to disable implicit transactions #350
Conversation
An sqlite3 database may be opened with the x-no-tx-wrap=true query parameter if the user would prefer to retain full control over the bounds of their transactions. With x-no-tx-wrap enabled, migrations should contain explicit BEGIN and COMMIT statements. This (optional) behaviour for the sqlite3 driver is consistent with the default behaviour for other drivers. Fixes #346.
Pull Request Test Coverage Report for Build 707
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and for writing the README!
database/sqlite3/sqlite3.go
Outdated
|
||
noTxWrap, err := strconv.ParseBool(qv.Get("x-no-tx-wrap")) | ||
if err != nil { | ||
noTxWrap = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's cleaner to return an error for invalid values. We should handle non-existent (e.g. unspecified) values separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did wonder about this, so I went looking for existing patterns. I found this behaviour in the cockroachdb driver. Would you still prefer an error -- at the cost of consistency -- in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's cleaner to fail for invalid inputs instead of silently assuming a value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed a commit with the change you requested. PTAL, @dhui.
Fixes #346.