Skip to content

Conversation

alexcwatt
Copy link
Contributor

@alexcwatt alexcwatt commented Oct 29, 2024

The SQLite docs recommend some cases where PRAGMA optimize; should be used: https://www.sqlite.org/pragma.html#pragma_optimize

I have discovered recently a case where this helped a project at work (https://twitter.com/alexcwatt/status/1851094243323912631) and now I'm wondering about how to make this easier to discover and use.

Do we want a way to test that these wrappers translate to the expected execute call? I wasn't sure what tests to write for this change.

@flavorjones
Copy link
Member

flavorjones commented Oct 30, 2024

@alexcwatt Thanks for this! The omission was mentioned by @rickhull in #559

If we're going to allow the optimize bits to be passed in by the caller, I'd like the API to be a bit more developer-friendly. Would you consider defining some constants in lib/sqlite3/constants.rb?

Our test coverage is pretty bad right now for the pragma methods, so as long as we keep the implementation simple, I think it's probably OK to not have test coverage (or to use a mock to test that the method calls what it should).

end

def optimize(bitmask = nil)
if bitmask
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see these bits made available as constants like many of the other sqlite bitmasks in lib/sqlite3/constants.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I took a pass at this; please let me know what you think.

@alexcwatt alexcwatt requested a review from flavorjones October 31, 2024 13:36
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.

Looks good! I added a DEFAULT constant and some tests.

@flavorjones
Copy link
Member

And I rebased off origin/main.

@flavorjones flavorjones enabled auto-merge October 31, 2024 16:00
@flavorjones flavorjones merged commit c5d1d5c into sparklemotion:main Oct 31, 2024
105 checks passed
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