Skip to content

Add support for all SQLite errors - #64

Open
joeybright wants to merge 5 commits into
gren-lang:mainfrom
joeybright:sqlite-errors
Open

joeybright wants to merge 5 commits into
gren-lang:mainfrom
joeybright:sqlite-errors

Conversation

@joeybright

Copy link
Copy Markdown
Contributor

This PR adds support for detecting all possible SQLite errors codes as documented on the SQLite website. The error types are meant to mimic the primary and extended code structure SQL uses to help organize errors & their possible reasons.

In addition, there's added support for detecting more generic Node errors which have the code "ERR_INVALID_STATE" and casting them as the Error type instead of UnknownError. It's unclear if there are additional node-specific errors that are not captured by this work and would otherwise be cast as the UnknownError type.

The errorToString function in the Sqlite module was removed, given the number of new errors that'd need unique strings results.

No additional tests have been added, given there's quite lot new errors, many of which I am not certain how to reliably produce. However, existing tests that expected specific types of errors match the new errors and needed only minor edits to conform to the new error type shape.

@blaix

blaix commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

I like the specificity of the errors and parity with sqlite. Not sure about losing errorToString though, seems like something people will commonly want for logging, etc. I know I already have several places where I'm using this. Is there a technical reason not to keep it other than it just being tedious to support now? I don't think you'd need it for each nested variant but seems reasonable for the top-level variants maybe? (not sure)

@blaix

blaix commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

I guess we should also start thinking about how we want to expose these errors from the outside world. For example, in FileSystem the error type is opaque with helper functions. What was the reasoning there @robinheghan? Was it that we have a much higher surface for breaking major changes with directly exposing them? if so, something to consider here...

Comment thread src/Gren/Kernel/Sqlite.js Outdated
Comment thread src/Gren/Kernel/Sqlite.js
Comment thread src/Sqlite.gren
Comment thread src/Sqlite.gren
@robinheghan

Copy link
Copy Markdown
Member

I agree with @blaix : we should keep errorToString. Super handy for logging.

What was the reasoning there @robinheghan?

Filesystem errors depends on the platform and filesystem, so making a complete list of errors seemed impossible to me at the time.

Sqlite is different, since sqlite itself tries to work the same across platforms, has a very stable API and has documented all their errors.

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.

3 participants