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

Provide the SQL statement context #554

Merged

Conversation

fractaledmind
Copy link
Contributor

for errors that occur when preparing a SQL statement

@flavorjones @tenderlove @byroot

This is a proof-of-concept PR to explore providing better context for exceptions raised. To begin, I focused on exceptions raised when attempting to prepare a statement. Using the gem over the years, there are other contexts that I found the error messages lacking, but I need to recall them and work on PoCs for each. But I wanted to open this to start the conversation around improving error messages with context.

Copy link
Member

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

This seems like a great feature to add, if we can clean it up a little bit as suggested by @byroot.

Also please note there's a failing test where we run with an older version of sqlite that doesn't have sqlite3_error_offset. To deal with older versions you'll need to add a have_func in the extconf and then #ifdef HAVE_SQLITE3_ERROR_OFFSET in the code. Let me know if you want any help!

ext/sqlite3/exception.c Outdated Show resolved Hide resolved
@fractaledmind
Copy link
Contributor Author

Thank you! I put this WIP PR out there after a quick "hacking session" while prepping talks. I'll get it cleaned up when I'm back from my grad conference adventures.

But, if you have a clear vision and a hacking session to push this over the finish line, I have no sense of ownership over this PR.

Either way, we can get this finished up in October at the latest.

ext/sqlite3/exception.c Outdated Show resolved Hide resolved
@flavorjones flavorjones force-pushed the preparation-error-context branch 4 times, most recently from 5e54fe2 to a2df246 Compare October 20, 2024 18:21
@flavorjones flavorjones force-pushed the preparation-error-context branch from a2df246 to d748d5e Compare October 20, 2024 18:25
@flavorjones
Copy link
Member

I moved the sql error message handling into Ruby and made it backwards-compatible with older versions of sqlite. Would love a bit of review on this if you have a moment @fractaledmind @tenderlove @byroot.

Copy link
Contributor

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

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

LGTM

@fractaledmind
Copy link
Contributor Author

Looks good to me too. The C code is indeed much better, so thank you all for improving my PoC so considerably. I fixed the merge conflict in the CHANGELOG, so hopefully all the checks pass now.

@flavorjones flavorjones merged commit 2d0139d into sparklemotion:main Oct 23, 2024
105 checks passed
@fractaledmind fractaledmind deleted the preparation-error-context branch October 23, 2024 16:21
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.

5 participants