bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#70145: [PATCH] Add sqlite-execute-batch command


From: Javier Olaechea
Subject: bug#70145: [PATCH] Add sqlite-execute-batch command
Date: Tue, 2 Apr 2024 21:42:06 -0500


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

Attachment: 0001-Add-sqlite-execute-batch-command.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]