Phil Estival <p...@7d.nz> writes: >> May you please update your latest patch for ob-sql.el, converting it >> into (1) patch to sql.el; (2) patch for ob-sql.el that assumes changes >> to sql.el? > > Here they are > 1) the patch of sql.el > 2) the list of changes in ob-sql.el against release_9.7.30. > The patch that assumes the change in sql.el is n°13.
Thanks! I will comment on the Org-related part. > Subject: [PATCH 01/14] ob-sql: session support. Introduces new variables and > functions > > * lisp/ob-sql.el: new variables and functions for session support for > postgres and sqlite. Requires sql.el for a connection to a session. > SQL clients are configured by a preamble of commands given to the SQL > shell. Custom variables are declared in a new sub-group ob-babel-sql. > The echo of an SQL ANSI comment is to be appended to the source block > of SQL commands for comint to detect when the commands terminate, > instead of relying on prompt detection. This is not a complete changelog of your changes. Please check out https://orgmode.org/worg/org-contribute.html#commit-messages and follow the format. In general, because you plan to be the maintainer, please familiarize yourself with all the conventions discussed in https://orgmode.org/worg/org-contribute.html and https://orgmode.org/worg/org-maintenance.html > ;; Author: Eric Schulte > ;; Maintainer: Daniel Kraus <dan...@kraus.my> > +;; Maintainer: Philippe Estival <p...@7d.nz> > ;; Keywords: literate programming, reproducible research > ;; URL: https://orgmode.org I think adding you as a maintainer should be a separate patch to make the change distinct. > (require 'ob) > +(require 'sql) Do we need to load sql.el early? May we do it dynamically instead? > +(defvar org-babel-sql-session-start-time) This variable is unused. What is it for? > +(defvar org-sql-session-preamble > + (list > + 'postgres "\\set ON_ERROR_STOP 1 > +\\pset footer off > +\\pset pager off > +\\pset format unaligned" ) > + "Command preamble to run upon shell start.") Should it be a defcustom? > +(defvar org-sql-session-command-terminated nil) AFAIU, this is some kind of flag to be used inside sql comit buffer. Should it be buffer-local? Also, please add a docstring. > +(defvar org-sql-session--batch-terminate "---#" "To print at the end of a > command batch.") This is a bit weird formatting. In Elisp, the docstring is usually placed on a separate line. Also, please check out https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html Your docstring here reads confusing. > +(defvar org-sql-batch-terminate > + (list 'sqlite (format ".print %s\n" org-sql-session--batch-terminate) > + 'postgres (format "\\echo %s\n" org-sql-session--batch-terminate)) > + "Print the command batch termination as last command.") This implies that you treat `org-sql-session--batch-terminate' as a constant. If so, maybe declare it as such. Also, should it be a defcustom? > +(defvar org-sql-terminal-command-prefix > + (list 'sqlite "\\." > + 'postgres "\\\\") > + "Identify a command for the SQL shell.") This variable is unused. > +(defvar org-sql-environment > + (list 'postgres '(("PGPASSWORD" sql-password)))) Also unused. > +(defvar org-sql-session-clean-output nil > + "Store the regexp used to clear output (prompt1|termination|prompt2).") Should it be buffer-local in session buffer? > +(defvar org-sql-session-start-time) > +(defvar org-sql-session-command-terminated nil) Same question. > +(defvar org-sql-session--batch-terminate "---#" "To print at the end of a > command batch.") Duplicate defvar. > +(defcustom org-sql-run-comint-p 'nil There is no need to quote nil and numbers. They are self-quoting. > + "Run non-session SQL commands through comint if not nil." > + :type '(boolean) :type 'boolean > +(defcustom org-sql-timeout '5.0 > + "Abort on timeout." > + :type '(number) :type 'number > +(defcustom org-sql-close-out-temp-buffer-p 'nil > + "To automatically close sql-out-temp buffer." The docstring is not very clear. > + :type '(boolean) :type 'boolean > +(defun org-sql-session-comint-output-filter (_proc string) > + "Process output STRING of PROC gets redirected to a temporary buffer. > +It is called several times consecutively as the shell outputs and flush > +its message buffer" > + > + ;; Inserting a result in the sql process buffer (to read it as a > + ;; regular prompt log) inserts it to the terminal, and as a result the > + ;; ouput would get passed as input onto the next command line; See > + ;; `comint-redirect-setup' to possibly fix that, > + ;; (with-current-buffer (process-buffer proc) (insert output)) > + > + (when (or (string-match org-sql-session--batch-terminate string) > + (> (time-to-seconds > + (time-subtract (current-time) > + org-sql-session-start-time)) > + org-sql-timeout)) > + (setq org-sql-session-command-terminated t)) > + > + (with-current-buffer (get-buffer-create "*ob-sql-result*") > + (insert string))) Using global `org-sql-session-command-terminated', `org-sql-session-start-time', and "*ob-sql-result*" buffer will not be reliable when there are multiple comint sessions. It will also make it impossible to implement async sessions. Please use some other approach (for example, buffer-local or process-local variables) > * lisp/ob-sql.el: removal of org-assert-version makes org-macs no > longer needed. > ... > - > -(require 'org-macs) > -(org-assert-version) > - (org-assert-version) is needed to detect mixed Org versions. (require 'org-macs) there is mostly to provide `org-assert-version'. > Subject: [PATCH 04/14] ob-sql: realign variables for improved readability. We do not allow whitespace-only commits. See https://orgmode.org/worg/org-contribute.html#orgbbd6fd6 > + (when colnames-p (with-temp-buffer nitpick: It is unusual to place (when condition body-line1 ...) The common practice is (when condition body) > + (setq org-sql-session-clean-output > + (plist-put org-sql-session-clean-output in-engine > + (concat "\\(" prompt-regexp "\\)" > + "\\|\\(" org-sql-session--batch-terminate > "\n\\)" > + (when prompt-cont-regexp > + (concat "\\|\\(" prompt-cont-regexp > "\\)")))))) This will be broken when multiple sessions for different engines are used in parallel. Please avoid global state. > + (let ((sql--buffer > + (org-babel-sql-session-connect in-engine params session))) Why `sql--bufer'? (double slash) It is not a variable sql.el uses. > + (with-current-buffer (get-buffer-create "*ob-sql-result*") > + (erase-buffer)) > + (setq org-sql-session-start-time (current-time)) > + (setq org-sql-session-command-terminated nil) > + > + (with-current-buffer (get-buffer sql--buffer) > + (process-send-string (current-buffer) > + (org-sql-session-format-query > + (org-babel-expand-body:sql body > params) > + in-engine)) > + (while (or (not org-sql-session-command-terminated) > + (> (time-to-seconds > + (time-subtract (current-time) > + org-sql-session-start-time)) > + org-sql-timeout)) > + (sleep-for 0.03)) > + ;; command finished, remove filter > + (set-process-filter (get-buffer-process sql--buffer) nil) This looks repetitive with the code in `org-sql-session-comint-output-filter'. Could it be possible to avoid code duplication? > + (when (not session-p) > + (comint-quit-subjob) > + ;; despite this quit signal, the process may not be finished > yet > + (let ((kill-buffer-query-functions nil)) > + (kill-this-buffer)))) Maybe we should wait until it is finished then? You can check `process-status'. > Subject: [PATCH 11/14] ob-sql: replace call to (intern engine) by the > previously declared in-engine > > TINYCHANGE TINYCHANGE is unnecessary after you get the copyright assignment. (which do you need to get) > + (insert-for-yank Why do you need `insert-for-yank'? -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
