search

crawshaw / sqlite

Go SQLite3 driver
ISC License
561 stars 67 forks source link

Export sqlitex errors to allow handling with errors.Is #120

Closed shabbyrobe closed 2 years ago

shabbyrobe commented 3 years ago

This fixes the issue raised in #99. I think just exporting these errors for use with errors.Is is the best approach as it doesn't require any build tag stuff to support the pre-errors.Is Go version declared in the go.mod.

anacrolix commented 3 years ago

Probably wouldn't hurt to throw in or modify a unit test or two to just check this works as intended?

shabbyrobe commented 3 years ago

Sure, I've added some tests for the Result functions now.

anacrolix commented 3 years ago

I think https://golang.org/pkg/testing/#T.Cleanup might replace the done funcs, and that SentenceCase is usual for test names. I don't know if T.Run handles multiple levels in a single call, you might want to do t.Run("Int64"), then nest within that more calls to t.Run, or do t.Run("Int64SomeNestedTest").

anacrolix commented 3 years ago

I see you likely avoided T.Cleanup and errors.Is due to the version directive, it would be great if the maintainers allowed that to be bumped to 1.14!

shabbyrobe commented 3 years ago

I have updated to sentence case as requested.

To your point about nesting, the go test function gives slashes a special meaning - go test -run patterns will actually split into a series of patterns on the slashes. This is a blessed alternative to deep nesting of t.Run calls and expresses the same hierarchy, albeit using a different mechanism. For example:

$ go test -v -run='Result/t/E'
=== RUN   TestResult
=== RUN   TestResult/Int64/ErrMultipleResults
=== RUN   TestResult/Int64/ErrNoResult
=== RUN   TestResult/Float/ErrMultipleResults
=== RUN   TestResult/Float/ErrNoResult
=== RUN   TestResult/Text/ErrMultipleResults
=== RUN   TestResult/Text/ErrNoResult
--- PASS: TestResult (0.00s)

Having said that, I'm not wed to this, and if you'd rather nested t.Run() calls, please let me know.