Implements stmt.named_params#642
Conversation
|
@flavorjones mind taking a look? |
8412fb0 to
c73faa9
Compare
flavorjones
left a comment
There was a problem hiding this comment.
@captn3m0 Thanks for this! I've kicked off CI.
I'd like to see a test for the behavior of this code when given ?NNN parameters. Do you think the method should return those parameters? I'm not sure what happens if you try to bind a variable to a name like ?13.
https://sqlite.org/forum/forumpost/533576942b17bf66 is the best explanation I could find.
Nothing special about 3 digits either, ?1 binds as the first param, and ?1111 happily binds as the 1111th parameter.
stmt = SQLite3::Statement.new(@db, "select ?1")
stmt.bind_param(1, "foo")
assert_equal ["1"], stmt.named_params
stmt.each do |row|
assert_equal ["foo"], row
endMy first thought is that these aren't really named parameters and we should just drop them. But then, if you were using a statement like: "select ?2, ?3", you'd be back to the same problem this PR is trying to solve for these One way would be to return a mixed array? [2, "foo", "bar", 3], so that the values in the array can be directly used against |
|
Spent a bit more time at the problem, and it looks like the And there's no way to differentiate and get a result that says [1,2,50].
I wrote this implementation, which works well as long as params are either numbered or named. Once you add anonymous params to the mix, it goes haywire. Suggestion: |
|
Pushed a change to ignore numeric and anonymous parameters: stmt = SQLite3::Statement.new(@db, "select ?1, :foo, ?, $bar, @zed, ?250, @999")
assert_equal ["foo", "bar", "zed"], stmt.named_params |
|
Looked at the windows CI failures, but couldn't figure out why. |
flavorjones
left a comment
There was a problem hiding this comment.
Please don't worry about the failing tests for windows and ruby 3.2, 3.3, and 3.4 -- they're failing for unrelated reasons.
I added some comments about support for named parameters that happen to be numeric.
This is looking good! I feel like it's really close, just the numeric edge case and I think we can merge it.
|
Thanks, will take a look and get back. |
|
Update the FAQ alongside the code as well. |
numeric unused params are undistinguishable from numeric bindable parameters. So a simple loop from 1-bind_parameter_count is the best you can do. This means .named_params can be focused on the truly named parameters only, which is what This commit does by dropping numeric parameters
Even in cases where VVV is a numeric value, these are considered named parameters by sqlite.
779d16c to
f414bbe
Compare
|
Rebased and added a commit with small changes to the docstring. I think this is ready to go, when it goes green I'll merge it. |
|
Merged! Will be in v2.9.0 when it's released. |
This is based on the pending release for sqlite-ruby Ref: sparklemotion/sqlite3-ruby#642
Ref: sparklemotion/sqlite3-ruby#642 Released in sqlite ruby gem 2.9.0
Ref: sparklemotion/sqlite3-ruby#642 Released in sqlite ruby gem 2.9.0
Ref: sparklemotion/sqlite3-ruby#642 Released in sqlite ruby gem 2.9.0
Closes #627
Implemented
named_paramsdirectly in C. Not a C programmer, mistakes are mine. Learned about@and$prefixes while reading the docs, so added them to the test as well.However, this implementation ignores
sqlite3_bind_parameter_indexentirely, which is only used internally so shouldn't be an issue(?). As of now, the index of the parameter in the result might not match the value returned bysqlite3_bind_parameter_indexin some cases.But this feels better than having to pad the result with nulls.
Once this is finalized, I can update the changelog if needed