Re: [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Stefano Rodighiero <stefano.rodighiero@gmail.com> writes:

> [This is the first patch I ever submitted.

Thank you. Some comments follow.

> I hope it complies with
> your standards: if it does not, I'll be happy to work on it until it's
> fine.  I am not sure it qualifies as a tiny change.]

It does.

> Subject: [PATCH] ob-sql.el: Option to reference connections in
>  `sql-connection-alist'
>
> * ob-sql.el: Provide a new param called :dbconnection, that can be
> used to reference connections defined in `sql-connection-alist', a
> custom variable defined in sql.el.

You need to spell out the new function in the commit message:

* ob-sql.el (org-babel-find-db-connection-param): new function.

This patch provides a new parameter :dbconnection, ...

> +(defun org-babel-find-db-connection-param (params name)
> +  "Return db connection param NAME.

database, parameter

> +Given a param NAME, if :dbconnection is defined in PARAMS then
> +look for the param into the corresponding connection defined in
> +`sql-connection-alist`, otherwise look into PARAMS.  Look
> +`sql-connection-alist` (part of SQL mode) for how to define
> +database connections."
> +  (if (assq :dbconnection params)
> +      (let* ((dbconnection (cdr (assq :dbconnection params)))
> +             (name-mapping '((dbhost . sql-server)
> +                             (dbport . sql-port)
> +                             (dbuser . sql-user)
> +                             (dbpassword . sql-password)
> +                             (database . sql-database)))
> +             (mapped-name (cdr (assq name name-mapping))))
> +        (cadr (assq mapped-name
> +                    (cdr (assoc dbconnection
> +                                sql-connection-alist)))))
> +    (cdr (assq name params))))

Isn't there a type mismatch here? 
> +
>  (defun org-babel-execute:sql (body params)
>    "Execute a block of Sql code with Babel.
>  This function is called by `org-babel-execute-src-block'."
>    (let* ((result-params (cdr (assq :result-params params)))
>           (cmdline (cdr (assq :cmdline params)))
> -         (dbhost (cdr (assq :dbhost params)))
> -         (dbport (cdr (assq :dbport params)))
> -         (dbuser (cdr (assq :dbuser params)))
> -         (dbpassword (cdr (assq :dbpassword params)))
> -         (database (cdr (assq :database params)))
> +         (dbhost (org-babel-find-db-connection-param params 'dbhost))
> +         (dbport (org-babel-find-db-connection-param params 'dbport))
> +         (dbuser (org-babel-find-db-connection-param params 'dbuser))
> +         (dbpassword (org-babel-find-db-connection-param params 'dbpassword))
> +         (database (org-babel-find-db-connection-param params 'database))
>           (engine (cdr (assq :engine params)))

The change from keywords to plain symbols is slightly confusing. Could
`org-babel-find-db-connection-param' use keywords instead? This is the
usual format for parameters in Babel.

Regards,

-- 
Nicolas Goaziou