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

Export raw SQLite result codes #261

Closed
wants to merge 1 commit into from

Conversation

cole-miller
Copy link
Contributor

Closes #259. My preference here is to export all these constants under their "C names", e.g. SQLITE_CONSTRAINT, to make their raw nature obvious, but I can switch this PR to Go-style names if @freeekanayaka or @MathieuBordere feel strongly about it.

Signed-off-by: Cole Miller cole.miller@canonical.com

@freeekanayaka
Copy link
Contributor

I'd probably use the same naming as go-sqlite3: https://github.com/mattn/go-sqlite3/blob/master/error.go, which is both more Go-ish and something more familiar for folks coming from go-sqlite3 itself.

@tomponline
Copy link
Member

That would be my preference too.

@cole-miller
Copy link
Contributor Author

I don't really object to changing the names, but note that go-sqlite3 uses the original C-style names for the integer constants (i.e. what we're exporting here) and Go-style names for vars that are wrapped in its special ErrNo type, which I guess exists just so that something can be returned that implements the Error interface. For go-dqlite we already have a custom Error type that fills that role, packing up both the integer result code and the string description; arguably by using the C-style names we'd be sticking closer to what go-sqlite3 does.

@freeekanayaka
Copy link
Contributor

I don't really object to changing the names, but note that go-sqlite3 uses the original C-style names for the integer constants

Do you mean these? https://github.com/mattn/go-sqlite3/blob/v1.14.17/sqlite3.go#L302

If so, it seems that go-sqlite3 uses C-style names only for some kinds of constants, but not for error constants specifically?

(i.e. what we're exporting here) and Go-style names for vars that are wrapped in its special ErrNo type, which I guess exists just so that something can be returned that implements the Error interface. For go-dqlite we already have a custom Error type that fills that role, packing up both the integer result code and the string description; arguably by using the C-style names we'd be sticking closer to what go-sqlite3 does.

FWIW we also already have Go-style names for some of the error constants:

const (
so one natural thing to do in my view would be to add the missing ones.

The way I see it, using Go-style names would be 1) consistent with what we already do 2) consistent with what go-sqlite3 does for error constants 3) consistent with Go naming convention in general.

I'd find it odd to have both driver.ErrBusy and driver.SQLITE_BUSY. In my mind it's actually kind of good to not have the SQLITE prefix, because the idea is that this error set is a super-set of SQLite errors, whose values and semantics is mostly the same but that can have subtle differences (which should be ideally documented, although they might not be at the moment).

@cole-miller
Copy link
Contributor Author

Thanks, you're right, I didn't read the API documentation for go-sqlite3 closely enough. I will switch the PR over.

Signed-off-by: Cole Miller <cole.miller@canonical.com>
@MathieuBordere
Copy link
Contributor

@cole-miller I'm okay to merge this, does this need anything more?

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.

Client Error Handling needs improvement
4 participants