Re: [PATCH] ob-sql: session

[Ihor Radchenko <yantar92@posteo.net>]

Phil Estival <pe@7d.nz> writes:

> Hello. Here we go again.

Thanks!
See comments inline.

> Also, in the commit message of the patch for the tests,
> I mention that some macros should probably be moved upward
> in a file where generic functions which purposes are to help
> writing the tests of babel source blocks should be declared
> (ob-src-testfuncs.el for instance).
>
> Examples :
> - result-should-contain (regexp block) : Checking that REGEXP(s)
>    matches the command executed when evaluating BLOCK.
> - result-should-not-contain (regexp block)
> - result-equals (str block) and so on.

I do not mind.
But please show which _other_ tests can benefit from the simplification.

> +(defvar ob-sql-session--batch-end-indicator  "---#"  "Indicate the end of a command batch.")
> +(defvar ob-sql-session-command-terminated nil)
> +(defvar org-babel-sql-out-file)
> +(defvar org-babel-sql-session-start-time)
> +
> +(sql-set-product-feature 'sqlite :prompt-regexp "sqlite> ")
> +(sql-set-product-feature 'sqlite :batch-terminate
> +                         (format ".print %s\n" ob-sql-session--batch-end-indicator))
> +(sql-set-product-feature 'sqlite :terminal-command "\\.")
> +
> +(sql-set-product-feature 'postgres :prompt-regexp "SQL> ")
> +(sql-set-product-feature 'postgres :prompt-cont-regexp "> ")
> +(sql-set-product-feature 'postgres :batch-terminate
> +                         (format "\\echo %s\n" ob-sql-session--batch-end-indicator))
> +(sql-set-product-feature 'postgres :terminal-command "\\\\")
> +(sql-set-product-feature 'postgres :environment '(("PGPASSWORD" sql-password)))
> +(sql-set-product-feature
> + 'postgres :sqli-options
> + (list "--set=ON_ERROR_STOP=1"
> +       (format "--set=PROMPT1=%s" (sql-get-product-feature 'postgres :prompt-regexp ))
> +       (format "--set=PROMPT2=%s" (sql-get-product-feature 'postgres :prompt-cont-regexp ))
> +       "-P" "pager=off"
> +       "-P" "footer=off"
> +       "-A" ))

All these `sql-set-product-feature' calls are overriding the defaults
from sql.el. They will not only affect Org babel blocks, but all the
interactive SQL sessions in Emacs. Such side effects are not acceptable.

May we somehow avoid modifying pre-existing sql features?

It is probably OK to add Org-specific settings after prepending them
with org-. For example, :batch-terminate -> :org-batch-terminate.
Although, I am not sure what is the benefit of storing these _new_
settings in the `sql-product-alist'.
  
> +(defun ob-sql-session-buffer-live-p (buffer)
> +  "Return non-nil if the process associated with buffer is live.
> +
> +This redefines `sql-buffer-live-p' of sql.el, considering the terminal
> +is valid even when `sql-interactive-mode' isn't set.  BUFFER can be a buffer
> +object or a buffer name.  The buffer must be a live buffer, have a
> +running process attached to it, and, if PRODUCT or CONNECTION are
> +specified, its `sql-product' or `sql-connection' must match."
> +
> +  (let ((buffer (get-buffer buffer)))
> +    (and buffer
> +         (buffer-live-p buffer)
> +         (let ((proc (get-buffer-process buffer)))
> +           (and proc (memq (process-status proc) '(open run)))))))

May you simply use `org-babel-comint-buffer-livep' instead?

> +(defun org-babel-sql-session-connect (in-engine params session)
> +  "Start the SQL client of IN-ENGINE if it has not.
> +PARAMS provides the sql connection parameters for a new or
> +existing SESSION.  Clear the intermediate buffer from previous
> +output, and set the process filter.  Return the comint process
> +buffer.
> +
> +The buffer naming was shortened from
> +*[session] engine://user@host/database*,
> +that clearly identifies the connexion from Emacs,
> +to *SQL [session]* in order to retrieve a session with its
> +name alone, the other parameters in the header args beeing
> +no longer needed while the session stays open."
> +  (sql-set-product in-engine)
> ...

Is there any specific reason why you are seemingly re-implementing what
`sql-product-interactive' does? May we re-use it instead?

-- 
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>