[-- 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
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 #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
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".