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

Prepare many statements from a single string. #59

Open
wants to merge 7 commits into
base: v1.x.x
Choose a base branch
from

Conversation

cyisfor
Copy link

@cyisfor cyisfor commented Jul 1, 2020

sqlite3_prepare_v2 provides a tail parameter, and I figured out the trick with it a while ago. It sets tail to the point that it stops being able to parse, which is helpful for showing where your error is in your SQL statement, but it's also helpful for parsing multiple SQL statements. sqlite3_prepare only prepares the first, then "fails" at the beginning of the second statement right after the semicolon, which lets one prepare many statements from a single string. So that's what I did. Hope you like it!

Cy added 7 commits July 1, 2020 07:36
This will offer a prepare on demand InputRange instead of returning an array of statements.
Also added some documentation about what the heck I'm doing subtracting tail from head and all.
Right, can't mutate the stringz. Also explaining why the restrictive assert in there.
Gotta make sure it works.
I don't recall how you return an interface to a struct...
Oh right. You don't return an interface. You just return the struct and it magically has the right methods! Also a little more finagling to get the iterator to return empty only after preparation has failed on the tail.
Just paranoid about whether PrepareMany works as an input range or not.
@@ -278,11 +278,31 @@ public:

The statement becomes invalid if the Database goes out of scope and is destroyed.
+/
Statement prepare(string sql)
Statement prepare(const string sql)
Copy link
Member

Choose a reason for hiding this comment

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

Not needed, a string implicitly converts to a const string and vice versa.

Comment on lines +59 to +79
struct PrepareMany {
Database db;
Statement current;
string sql_left;
bool empty;
this(Database db, const string sql) {
this.db = db;
this.sql_left = sql;
popFront();
}
void popFront() {
const size_t old = sql_left.length;
current = Statement(this.db, sql_left);
empty = old == sql_left.length;
sql_left = sql_left.chomp();
}
Statement front() {
return current;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This is lacking visibility attributes (private / package) and attributes (@safe / nothrow / ...)

Comment on lines +317 to +321
unittest {
auto db = Database(":memory:");
db.execute("CREATE TABLE test (val INTEGER)");
auto statements =
db.prepare_many(q"[
Copy link
Member

Choose a reason for hiding this comment

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

I didn't see this was a unittest at first, please fix the alignment

Comment on lines +88 to +99
this(Database db, const string sql)
{
string temp = sql;
this(db, temp);
// this constructor won't be able to give any indication that
// the sql wasn't fully parsed, so make sure it is.
assert(temp.length == 0);
// the second constructor moves the sql parameter to the
// unparsed tail, so use that if you might be preparing 2 statements
}

this(Database db, ref string sql)
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand the point of this change. Why complicate the API this much ?

@Geod24
Copy link
Member

Geod24 commented Jul 29, 2020

It looks like a lot could be simplified and built purely on top of the existing Statement primitive. Also you should squash the commits.

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.

None yet

2 participants