GNU bug report logs - #70145
[PATCH] Add sqlite-execute-batch command

Previous Next

To reply to this bug, email your comments to 70145 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Javier Olaechea <pirata <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add sqlite-execute-batch command
Date: Tue, 2 Apr 2024 10:03:30 -0500

[Message part 1 (text/plain, inline)]

Hi, while writing tests for an Emacs package I found myself needing to
execute multiple statements against an in-memory database, to initialize
the schema. Currently there is no easy way to do so as sqlite-execute
only executes the first command and ignores the rest. The reason most
likely being that accepting arguments for multiple statements and
properly preparing would be a tricky task. So instead I'm adding the
functionality as a new function that takes no arguments.


In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
version 1.18.0, Xaw3d scroll bars)
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Ubuntu 20.04.6 LTS

Configured using:
 'configure
 --prefix=/nix/store/0g4xxdsn4xp9qhgc4cylbksqpwsn51vc-emacs-29.1
 --disable-build-details --with-modules --with-x-toolkit=lucid
 --with-xft --with-cairo --with-native-compilation --with-tree-sitter
 --with-xinput2'

-- 
"I object to doing things that computers can do." — Olin Shivers

[Message part 2 (text/html, inline)]

[0001-Add-sqlite-execute-batch-command.patch (text/x-patch, attachment)]

Message #8 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Javier Olaechea <pirata <at> gmail.com>
Cc: 70145 <at> debbugs.gnu.org
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Tue, 02 Apr 2024 19:21:52 +0300

> From: Javier Olaechea <pirata <at> gmail.com>
> Date: Tue, 2 Apr 2024 10:03:30 -0500
> 
> Hi, while writing tests for an Emacs package I found myself needing to
> execute multiple statements against an in-memory database, to initialize
> the schema. Currently there is no easy way to do so as sqlite-execute
> only executes the first command and ignores the rest. The reason most
> likely being that accepting arguments for multiple statements and
> properly preparing would be a tricky task. So instead I'm adding the
> functionality as a new function that takes no arguments.

Thanks.

> +DEFUN ("sqlite-execute-batch", Fsqlite_execute_batch, Ssqlite_execute_batch, 2, 2, 0,
> +       doc: /* Execute multiple SQL statements.  */)
> +  (Lisp_Object db, Lisp_Object query)
> +{
> +  check_sqlite (db, false);
> +  CHECK_STRING (query);
> +  return sqlite_exec (XSQLITE (db)->db, SSDATA (query));
> +}
> +

This has a subtle bug: it will only work correctly for plain-ASCII
string in QUERY.  If QUERY is allowed to included non-ASCII
characters, you need to encode it using encode_string.

Also, the doc string should mention the function's arguments, and it
should say what kind of Lisp object is QUERY.

> +@defun sqlite-execute-batch db statements
> +Execute the @acronym{SQL} @var{statements}. This might be useful when we
> +want to execute multiple @acronym{DDL} statements.

Same here, and in addition please leave two spaces between sentences,
per our conventions (in the commit log message as well).

Message #11 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Javier Olaechea <pirata <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70145 <at> debbugs.gnu.org
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Tue, 2 Apr 2024 13:52:12 -0500

[Message part 1 (text/plain, inline)]

Thanks, I've updated the patch according to the feedback.

Btw I have a basic test for the functionality which I used to validate the
code before submitting the patch. Should I include it in
test/sqlite-tests.el?

-- 
"I object to doing things that computers can do." — Olin Shivers

[Message part 2 (text/html, inline)]

[0001-Add-sqlite-execute-batch-command.patch (text/x-patch, attachment)]

[smoke.el (text/x-emacs-lisp, attachment)]

Message #14 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Javier Olaechea <pirata <at> gmail.com>
Cc: 70145 <at> debbugs.gnu.org
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Tue, 02 Apr 2024 21:56:01 +0300

> From: Javier Olaechea <pirata <at> gmail.com>
> Date: Tue, 2 Apr 2024 13:52:12 -0500
> Cc: 70145 <at> debbugs.gnu.org
> 
> Thanks, I've updated the patch according to the feedback. 

Thanks, will review soon.

> Btw I have a basic test for the functionality which I used to validate the code before submitting the patch.
> Should I include it in test/sqlite-tests.el?

Yes, more tests are always welcome.

Message #17 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Javier Olaechea <pirata <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70145 <at> debbugs.gnu.org
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Tue, 2 Apr 2024 14:10:15 -0500

[Message part 1 (text/plain, inline)]

Ok, updated the patch to include the test

On Tue, Apr 2, 2024 at 1:56 PM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Javier Olaechea <pirata <at> gmail.com>
> > Date: Tue, 2 Apr 2024 13:52:12 -0500
> > Cc: 70145 <at> debbugs.gnu.org
> >
> > Thanks, I've updated the patch according to the feedback.
>
> Thanks, will review soon.
>
> > Btw I have a basic test for the functionality which I used to validate
> the code before submitting the patch.
> > Should I include it in test/sqlite-tests.el?
>
> Yes, more tests are always welcome.
>


-- 
"I object to doing things that computers can do." — Olin Shivers

[Message part 2 (text/html, inline)]

[0001-Add-sqlite-execute-batch-command.patch (text/x-patch, attachment)]

Message #20 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: Javier Olaechea <pirata <at> gmail.com>
Cc: 70145 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Tue, 02 Apr 2024 18:56:54 -0700

Hi Javier,

Thanks for working on this. I think Emacs applications will find it
handy for things like initializing a new database. Just a couple things
I spotted in passing. (I don't really know anything, though, so feel
free to ignore.)

Javier Olaechea <pirata <at> gmail.com> writes:

> * test/lisp/sqlite-tests.el (with-sqlite-execute-batch-test): Test it.
         ~~~~

I think there's also a test/src/sqlite-tests.el file, which seems to
contain tests for those primitives defined in src/sqlite.c. Perhaps this
test should go there instead?

> diff --git a/test/lisp/sqlite-tests.el b/test/lisp/sqlite-tests.el
> index d4892a27efc..7053026eb82 100644
> --- a/test/lisp/sqlite-tests.el
> +++ b/test/lisp/sqlite-tests.el
> @@ -48,4 +48,29 @@ with-sqlite-transaction/rollback
>      ;; First insertion (a=1) rolled back.
>      (should-not (sqlite-select db "select * from test"))))
>  
> +(ert-deftest with-sqlite-execute-batch-test ()

Maybe I'm mistaken, but I don't think you need the "with-" part (or the
"-test" part), no?

Message #23 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Javier Olaechea <pirata <at> gmail.com>
To: "J.P." <jp <at> neverwas.me>
Cc: 70145 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Tue, 2 Apr 2024 21:42:06 -0500

[Message part 1 (text/plain, inline)]

> Thanks for working on this. I think Emacs applications will find it
> handy for things like initializing a new database. Just a couple things
> I spotted in passing. (I don't really know anything, though, so feel
> free to ignore.)
>

Feedback is always welcome. It's not like I know anything either ^_^'.


> I think there's also a test/src/sqlite-tests.el file, which seems to
> contain tests for those primitives defined in src/sqlite.c. Perhaps this
> test should go there instead?
>
>
Thanks for pointing that out, I hadn't seen that file. It does seem like a
better home for the test.


> > diff --git a/test/lisp/sqlite-tests.el b/test/lisp/sqlite-tests.el
> > index d4892a27efc..7053026eb82 100644
> > --- a/test/lisp/sqlite-tests.el
> > +++ b/test/lisp/sqlite-tests.el
> > @@ -48,4 +48,29 @@ with-sqlite-transaction/rollback
> >      ;; First insertion (a=1) rolled back.
> >      (should-not (sqlite-select db "select * from test"))))
> >
> > +(ert-deftest with-sqlite-execute-batch-test ()
>
> Maybe I'm mistaken, but I don't think you need the "with-" part (or the
> "-test" part), no?
>

I did find it contrary to the foo-test- prefix I've seen in the elisp code
in the wild, but on matters of naming conventions I try to follow the
motto: "When in Rome, do as Romans do". So I named the test according to
the other tests in the file. Now that I placed it under
test/src/sqlite-tests.el I've changed the name to align with the names of
the test in that file.

I've attached a new patch incorporating your feedback

-- 
"I object to doing things that computers can do." — Olin Shivers

[Message part 2 (text/html, inline)]

[0001-Add-sqlite-execute-batch-command.patch (text/x-patch, attachment)]

Message #26 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Javier Olaechea <pirata <at> gmail.com>
Cc: 70145 <at> debbugs.gnu.org, jp <at> neverwas.me
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Sat, 13 Apr 2024 11:03:53 +0300

> From: Javier Olaechea <pirata <at> gmail.com>
> Date: Tue, 2 Apr 2024 21:42:06 -0500
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 70145 <at> debbugs.gnu.org
> 
>  Thanks for working on this. I think Emacs applications will find it
>  handy for things like initializing a new database. Just a couple things
>  I spotted in passing. (I don't really know anything, though, so feel
>  free to ignore.)
> 
> Feedback is always welcome. It's not like I know anything either ^_^'.
>  
>  I think there's also a test/src/sqlite-tests.el file, which seems to
>  contain tests for those primitives defined in src/sqlite.c. Perhaps this
>  test should go there instead?
> 
> Thanks for pointing that out, I hadn't seen that file. It does seem like a better home for the test.
>  
>  > diff --git a/test/lisp/sqlite-tests.el b/test/lisp/sqlite-tests.el
>  > index d4892a27efc..7053026eb82 100644
>  > --- a/test/lisp/sqlite-tests.el
>  > +++ b/test/lisp/sqlite-tests.el
>  > @@ -48,4 +48,29 @@ with-sqlite-transaction/rollback
>  >      ;; First insertion (a=1) rolled back.
>  >      (should-not (sqlite-select db "select * from test"))))
>  >  
>  > +(ert-deftest with-sqlite-execute-batch-test ()
> 
>  Maybe I'm mistaken, but I don't think you need the "with-" part (or the
>  "-test" part), no?
> 
> I did find it contrary to the foo-test- prefix I've seen in the elisp code in the wild, but on matters of naming
> conventions I try to follow the motto: "When in Rome, do as Romans do". So I named the test according to the
> other tests in the file. Now that I placed it under test/src/sqlite-tests.el I've changed the name to align with the
> names of the test in that file.
> 
> I've attached a new patch incorporating your feedback

Thanks.  This should have a NEWS entry announcing the new command.

Also, to accept this contribution, we'd need a copyright assignment
from you.  Would you like to start the paperwork for that at this
time?  If yes, I will send you the form to fill and the instructions
to go with it.

Message #29 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Javier Olaechea <pirata <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70145 <at> debbugs.gnu.org, jp <at> neverwas.me
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Sat, 13 Apr 2024 22:32:05 -0500

[Message part 1 (text/plain, inline)]

> Thanks.  This should have a NEWS entry announcing the new command
>

Updated the patch to include the NEWS entry.


> Also, to accept this contribution, we'd need a copyright assignment
> from you.  Would you like to start the paperwork for that at this
> time?  If yes, I will send you the form to fill and the instructions
> to go with it.
>

Yes, please send the form please.

-- 
"I object to doing things that computers can do." — Olin Shivers

[Message part 2 (text/html, inline)]

[0001-Add-sqlite-execute-batch-command.patch (text/x-patch, attachment)]

Message #32 received at 70145 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Javier Olaechea <pirata <at> gmail.com>
Cc: 70145 <at> debbugs.gnu.org, jp <at> neverwas.me
Subject: Re: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Sun, 14 Apr 2024 08:52:36 +0300

> From: Javier Olaechea <pirata <at> gmail.com>
> Date: Sat, 13 Apr 2024 22:32:05 -0500
> Cc: jp <at> neverwas.me, 70145 <at> debbugs.gnu.org
> 
>  Also, to accept this contribution, we'd need a copyright assignment
>  from you.  Would you like to start the paperwork for that at this
>  time?  If yes, I will send you the form to fill and the instructions
>  to go with it.
> 
> Yes, please send the form please.

Form sent off-list.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.