[PATCH] Reduce code duplication in ob-sql.el and ob-sqlite.el

[PATCH] Reduce code duplication in ob-sql.el and ob-sqlite.el

[Nick Savage]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

Hi everyone,

See the attached patch. It is a small change to reduce code duplication 
between ob-sql.el and ob-sqlite.el by reusing org-babel-sql-expand-vars 
as suggested by the FIXME in ob-sqlite.el.

Thanks,

Nick


[-- Attachment #2: 0001-Reduce-code-duplication-in-ob-sqlite.el-and-ob-sql.e.patch --]
[-- Type: text/x-patch, Size: 3616 bytes --]

From 849a1b7417b39abbf0aeb1b241e890470e4111cd Mon Sep 17 00:00:00 2001
From: Nicholas Savage <nick@nicksavage.ca>
Date: Wed, 3 Mar 2021 07:47:15 -0500
Subject: [PATCH] Reduce code duplication in ob-sqlite.el and ob-sql.el

* lisp/ob-sqlite.el (org-babel-sqlite-expand-vars): removed function
to replace with ob-sql.el version
* lisp/ob-sql.el (org-babel-sql-expand-vars): updated to support
expanding sqlite vars
---
 lisp/ob-sql.el    | 15 +++++++++++----
 lisp/ob-sqlite.el | 24 +++---------------------
 2 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/lisp/ob-sql.el b/lisp/ob-sql.el
index 68d5ddd0a..b1c6920cb 100644
--- a/lisp/ob-sql.el
+++ b/lisp/ob-sql.el
@@ -350,8 +350,13 @@ SET COLSEP '|'
 	 (org-babel-pick-name (cdr (assq :rowname-names params))
 			      (cdr (assq :rownames params))))))))
 
-(defun org-babel-sql-expand-vars (body vars)
-  "Expand the variables held in VARS in BODY."
+(defun org-babel-sql-expand-vars (body vars &optional sqlite)
+  "Expand the variables held in VARS in BODY.
+
+If SQLITE has been provided, prevent passing a format to
+`orgtbl-to-csv'.  This prevents overriding the default format, which if
+there were commas in the context of the table broke the table as an
+argument mechanism."
   (mapc
    (lambda (pair)
      (setq body
@@ -362,9 +367,11 @@ SET COLSEP '|'
                   (let ((data-file (org-babel-temp-file "sql-data-")))
                     (with-temp-file data-file
                       (insert (orgtbl-to-csv
-                               val '(:fmt (lambda (el) (if (stringp el)
+                               val (if sqlite
+                                       nil
+                                     '(:fmt (lambda (el) (if (stringp el)
                                                       el
-                                                    (format "%S" el)))))))
+                                                    (format "%S" el))))))))
                     data-file)
                 (if (stringp val) val (format "%S" val))))
 	    body)))
diff --git a/lisp/ob-sqlite.el b/lisp/ob-sqlite.el
index 6e21fa9fd..2ec9deb78 100644
--- a/lisp/ob-sqlite.el
+++ b/lisp/ob-sqlite.el
@@ -27,6 +27,7 @@
 
 ;;; Code:
 (require 'ob)
+(require 'ob-sql)
 
 (declare-function org-table-convert-region "org-table"
 		  (beg0 end0 &optional separator))
@@ -51,8 +52,8 @@
 
 (defun org-babel-expand-body:sqlite (body params)
   "Expand BODY according to the values of PARAMS."
-  (org-babel-sqlite-expand-vars
-   body (org-babel--get-vars params)))
+  (org-babel-sql-expand-vars
+   body (org-babel--get-vars params) t))
 
 (defvar org-babel-sqlite3-command "sqlite3")
 
@@ -110,25 +111,6 @@ This function is called by `org-babel-execute-src-block'."
 	   (org-babel-sqlite-offset-colnames
 	    (org-table-to-lisp) headers-p)))))))
 
-(defun org-babel-sqlite-expand-vars (body vars)
-  "Expand the variables held in VARS in BODY."
-  ;; FIXME: Redundancy with org-babel-sql-expand-vars!
-  (mapc
-   (lambda (pair)
-     (setq body
-	   (replace-regexp-in-string
-	    (format "$%s" (car pair))
-	    (let ((val (cdr pair)))
-              (if (listp val)
-                  (let ((data-file (org-babel-temp-file "sqlite-data-")))
-                    (with-temp-file data-file
-                      (insert (orgtbl-to-csv val nil)))
-                    data-file)
-                (if (stringp val) val (format "%S" val))))
-	    body)))
-   vars)
-  body)
-
 (defun org-babel-sqlite-table-or-scalar (result)
   "If RESULT looks like a trivial table, then unwrap it."
   (if (and (equal 1 (length result))
-- 
2.20.1

Re: [PATCH] Reduce code duplication in ob-sql.el and ob-sqlite.el

[Kyle Meyer]

Nick Savage writes:

> Hi everyone,
>
> See the attached patch. It is a small change to reduce code duplication 
> between ob-sql.el and ob-sqlite.el by reusing org-babel-sql-expand-vars 
> as suggested by the FIXME in ob-sqlite.el.

Thank you.  Looks good, though I think it'd be nice to keep
org-babel-sqlite-expand-vars around for a bit, marked as obsolete.

> Subject: [PATCH] Reduce code duplication in ob-sqlite.el and ob-sql.el
>
> * lisp/ob-sqlite.el (org-babel-sqlite-expand-vars): removed function
> to replace with ob-sql.el version
> * lisp/ob-sql.el (org-babel-sql-expand-vars): updated to support
> expanding sqlite vars

Please capitalize the first word after ":" and end the entries with a
period.

  https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html

> -(defun org-babel-sqlite-expand-vars (body vars)
> -  "Expand the variables held in VARS in BODY."
> -  ;; FIXME: Redundancy with org-babel-sql-expand-vars!
> -  (mapc
> -   (lambda (pair)
> -     (setq body
> -	   (replace-regexp-in-string
> -	    (format "$%s" (car pair))
> -	    (let ((val (cdr pair)))
> -              (if (listp val)
> -                  (let ((data-file (org-babel-temp-file "sqlite-data-")))
> -                    (with-temp-file data-file
> -                      (insert (orgtbl-to-csv val nil)))
> -                    data-file)
> -                (if (stringp val) val (format "%S" val))))
> -	    body)))
> -   vars)
> -  body)
> -

How about marking this with (declare (obsolete ...)) and keeping it
around as a wrapper that calls org-babel-sql-expand-vars?  That will
give any third-party code that may have used this for whatever reason
(perhaps unlikely) a chance to update.

Re: [PATCH] Reduce code duplication in ob-sql.el and ob-sqlite.el

[Nick Savage]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

Please see the attached updated patch with the changes requested.

Nick

On 3/4/21 12:25 AM, Kyle Meyer wrote:
> Nick Savage writes:
>
>> Hi everyone,
>>
>> See the attached patch. It is a small change to reduce code duplication
>> between ob-sql.el and ob-sqlite.el by reusing org-babel-sql-expand-vars
>> as suggested by the FIXME in ob-sqlite.el.
> Thank you.  Looks good, though I think it'd be nice to keep
> org-babel-sqlite-expand-vars around for a bit, marked as obsolete.
>
>> Subject: [PATCH] Reduce code duplication in ob-sqlite.el and ob-sql.el
>>
>> * lisp/ob-sqlite.el (org-babel-sqlite-expand-vars): removed function
>> to replace with ob-sql.el version
>> * lisp/ob-sql.el (org-babel-sql-expand-vars): updated to support
>> expanding sqlite vars
> Please capitalize the first word after ":" and end the entries with a
> period.
>
>    https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html
>
>> -(defun org-babel-sqlite-expand-vars (body vars)
>> -  "Expand the variables held in VARS in BODY."
>> -  ;; FIXME: Redundancy with org-babel-sql-expand-vars!
>> -  (mapc
>> -   (lambda (pair)
>> -     (setq body
>> -	   (replace-regexp-in-string
>> -	    (format "$%s" (car pair))
>> -	    (let ((val (cdr pair)))
>> -              (if (listp val)
>> -                  (let ((data-file (org-babel-temp-file "sqlite-data-")))
>> -                    (with-temp-file data-file
>> -                      (insert (orgtbl-to-csv val nil)))
>> -                    data-file)
>> -                (if (stringp val) val (format "%S" val))))
>> -	    body)))
>> -   vars)
>> -  body)
>> -
> How about marking this with (declare (obsolete ...)) and keeping it
> around as a wrapper that calls org-babel-sql-expand-vars?  That will
> give any third-party code that may have used this for whatever reason
> (perhaps unlikely) a chance to update.

[-- Attachment #2: 0001-Reduce-code-duplication-in-ob-sqlite.el-and-ob-sql.e.patch --]
[-- Type: text/x-patch, Size: 3595 bytes --]

From 1e2816c89a4fc87a8d01c20dfb5a3c4cf794b553 Mon Sep 17 00:00:00 2001
From: Nicholas Savage <nick@nicksavage.ca>
Date: Wed, 3 Mar 2021 07:47:15 -0500
Subject: [PATCH] Reduce code duplication in ob-sqlite.el and ob-sql.el

* lisp/ob-sqlite.el (org-babel-sqlite-expand-vars): Marked function as obsolete.
* lisp/ob-sql.el (org-babel-sql-expand-vars): Updated to support
expanding sqlite vars.
---
 lisp/ob-sql.el    | 15 +++++++++++----
 lisp/ob-sqlite.el | 23 +++++------------------
 2 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/lisp/ob-sql.el b/lisp/ob-sql.el
index 68d5ddd0a..b1c6920cb 100644
--- a/lisp/ob-sql.el
+++ b/lisp/ob-sql.el
@@ -350,8 +350,13 @@ SET COLSEP '|'
 	 (org-babel-pick-name (cdr (assq :rowname-names params))
 			      (cdr (assq :rownames params))))))))
 
-(defun org-babel-sql-expand-vars (body vars)
-  "Expand the variables held in VARS in BODY."
+(defun org-babel-sql-expand-vars (body vars &optional sqlite)
+  "Expand the variables held in VARS in BODY.
+
+If SQLITE has been provided, prevent passing a format to
+`orgtbl-to-csv'.  This prevents overriding the default format, which if
+there were commas in the context of the table broke the table as an
+argument mechanism."
   (mapc
    (lambda (pair)
      (setq body
@@ -362,9 +367,11 @@ SET COLSEP '|'
                   (let ((data-file (org-babel-temp-file "sql-data-")))
                     (with-temp-file data-file
                       (insert (orgtbl-to-csv
-                               val '(:fmt (lambda (el) (if (stringp el)
+                               val (if sqlite
+                                       nil
+                                     '(:fmt (lambda (el) (if (stringp el)
                                                       el
-                                                    (format "%S" el)))))))
+                                                    (format "%S" el))))))))
                     data-file)
                 (if (stringp val) val (format "%S" val))))
 	    body)))
diff --git a/lisp/ob-sqlite.el b/lisp/ob-sqlite.el
index 6e21fa9fd..332a29872 100644
--- a/lisp/ob-sqlite.el
+++ b/lisp/ob-sqlite.el
@@ -27,6 +27,7 @@
 
 ;;; Code:
 (require 'ob)
+(require 'ob-sql)
 
 (declare-function org-table-convert-region "org-table"
 		  (beg0 end0 &optional separator))
@@ -51,8 +52,8 @@
 
 (defun org-babel-expand-body:sqlite (body params)
   "Expand BODY according to the values of PARAMS."
-  (org-babel-sqlite-expand-vars
-   body (org-babel--get-vars params)))
+  (org-babel-sql-expand-vars
+   body (org-babel--get-vars params) t))
 
 (defvar org-babel-sqlite3-command "sqlite3")
 
@@ -112,22 +113,8 @@ This function is called by `org-babel-execute-src-block'."
 
 (defun org-babel-sqlite-expand-vars (body vars)
   "Expand the variables held in VARS in BODY."
-  ;; FIXME: Redundancy with org-babel-sql-expand-vars!
-  (mapc
-   (lambda (pair)
-     (setq body
-	   (replace-regexp-in-string
-	    (format "$%s" (car pair))
-	    (let ((val (cdr pair)))
-              (if (listp val)
-                  (let ((data-file (org-babel-temp-file "sqlite-data-")))
-                    (with-temp-file data-file
-                      (insert (orgtbl-to-csv val nil)))
-                    data-file)
-                (if (stringp val) val (format "%S" val))))
-	    body)))
-   vars)
-  body)
+  (declare (obsolete "use `org-babel-sql-expand-vars' instead." "Org 9.4"))
+  (org-babel-sql-expand-vars body vars t))
 
 (defun org-babel-sqlite-table-or-scalar (result)
   "If RESULT looks like a trivial table, then unwrap it."
-- 
2.20.1

Re: [PATCH] Reduce code duplication in ob-sql.el and ob-sqlite.el

[Kyle Meyer]

Nick Savage writes:

> Please see the attached updated patch with the changes requested.

Thanks.  Pushed (37749c165).

> +  (declare (obsolete "use `org-babel-sql-expand-vars' instead." "Org 9.4"))

... changing "Org 9.4" to "Org 9.5".