Re: [PATCH] ob-sql: session + sql.el w/o prompt

[Ihor Radchenko]

Phil Estival <pe@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 <daniel@kraus.my>
> +;; Maintainer: Philippe Estival <pe@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>