Skip to content

Conversation

tuxlinuxien
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jun 9, 2018

Coverage Status

Coverage increased (+0.06%) to 55.428% when pulling 474876b on tuxlinuxien:master into 4218441 on mattn:master.

@gjrtimmer
Copy link
Collaborator

@tuxlinuxien will try and test it today. Any other new features we should be looking into ?

@tuxlinuxien
Copy link
Contributor Author

@gjrtimmer the only thing I am aware of is the UPSERT feature. However is page is still marked as draft so maybe more official features will be announced later.

@gjrtimmer
Copy link
Collaborator

@tuxlinuxien Because this in an update of sqlite we probably also should look into the new Auxiliary columns. Do you think we need to update the virtual table and testing to support this out-of-the-box ?

@tuxlinuxien
Copy link
Contributor Author

Do you think we need to update the virtual table and testing to support this out-of-the-box ?

@gjrtimmer to be honest my knowledge are limited about the core of sqlite3 so I am not the right person to answer this question. Sorry

Copy link
Collaborator

@gjrtimmer gjrtimmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, one small change requested

sqlite3_test.go Outdated
func TestUpsert(t *testing.T) {
_, n, _ := Version()
if !(n >= 3024000) {
t.Log("Your version of sqlite3 doesn't support UPSERT feature.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace line 237-240 (including the return) with.

t.Skip("UPSERT requires sqlite3 => 3.24.0")

Reason: Output of the test results.

@gjrtimmer
Copy link
Collaborator

@tuxlinuxien Not a problem, I think UPSERT is the most important one, which people are waiting for.
The auxiliary columns feature can be implemented later on.

I've requested one change to your code, which is solid and very nice. Change I've requested is to limit the output lines of the test when its skipped.

@tuxlinuxien
Copy link
Contributor Author

@gjrtimmer done. Just need to wait for travis to check this up.

$>go test --tags='libsqlite3' -v ./. # local libsqlite3 3.22.0
[...]
=== RUN   TestUpsert
--- SKIP: TestUpsert (0.00s)
	sqlite3_test.go:237: UPSERT requires sqlite3 => 3.24.0
[...]

@gjrtimmer
Copy link
Collaborator

@tuxlinuxien Will merge after Travis-CI

@gjrtimmer gjrtimmer merged commit d31a44a into mattn:master Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants