[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [O] [PATCH] ob-sql.el: Option to reference connections in `sql-conne
From: |
Nicolas Goaziou |
Subject: |
Re: [O] [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist' |
Date: |
Sun, 07 Apr 2019 09:24:26 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Hello,
Stefano Rodighiero <address@hidden> 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
- Re: [O] [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist',
Nicolas Goaziou <=