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

Try to simplify pragma related constants #621

Merged
merged 3 commits into from
Mar 7, 2025
Merged

Try to simplify pragma related constants #621

merged 3 commits into from
Mar 7, 2025

Conversation

tenderlove
Copy link
Member

This PR is just trying to simplify pragma related constants. I guess these constants are used as a kind of lookup table. I added some regression tests because I saw we're not exercising all cases in the test suite, then refactored the constants to just be hash tables.

I left the very strange array search in set_enum_pragma for backwards compatibility, though I think we should deprecate it.

AFAICT, there are two types of pragmas. One uses integers, the other strings. I think if we changed some of the pragma APIs, we could delete the hash tables that are simply duplicated strings, but I'll do that in a follow up PR.

This commit just adds some test cases around the pragma APIs
This commit just simplifies the PRAGMA related constants
@flavorjones
Copy link
Member

@tenderlove Looks like there's a related test failure in the downstream activerecord test suite, I haven't dug into it yet and probably won't get a chance today.

@tenderlove
Copy link
Member Author

tenderlove commented Mar 7, 2025

@tenderlove Looks like there's a related test failure in the downstream activerecord test suite, I haven't dug into it yet and probably won't get a chance today.

I tracked it down to this, and I think I've fixed it in the latest commit.

Rails was testing that if it passes an invalid pragma value, we get an exception. It seems like our code (the SQLite3 Ruby bindings) are the things validating what pragmas are valid. I don't like that we're in the business of validating these because if SQLite3 ever changes then we also have to update our code. Not to mention, some pragmas might be valid on some versions of SQLite3 and not on others. I feel we should take a "garbage in garbage out" approach here, though that might mean a major version bump. I've made the code backwards compatible for now though.

@flavorjones
Copy link
Member

I think I've fixed it in the latest commit

Yup, it's green! Whacking auto-merge.

I don't like that we're in the business of validating these

Agree.

@flavorjones flavorjones enabled auto-merge March 7, 2025 21:20
@flavorjones flavorjones disabled auto-merge March 7, 2025 21:20
@flavorjones flavorjones enabled auto-merge March 7, 2025 21:21
@flavorjones flavorjones merged commit 90937d3 into main Mar 7, 2025
136 checks passed
@flavorjones flavorjones deleted the regression branch March 7, 2025 21:25
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.

2 participants