bug#24646: [PATCH] Complete the name of PostgreSQL databases

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 223 bytes --]

Hi.

I recently started using sql.el with PostgreSQL, and thought it would
be nice if `sql-postgres' would provide completion of available
PostgreSQL databases.

Is something like the attached patch sufficient?

-- Simen



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Complete-the-name-of-PostgreSQL-databases.patch --]
[-- Type: text/x-patch, Size: 4757 bytes --]

From 564096195e41ef00e09fbfbaadfe8c7bf769773f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Thu, 6 Oct 2016 20:42:53 +0200
Subject: [PATCH] Complete the name of PostgreSQL databases

* lisp/progmodes/sql.el (sql-postgres-login-params): Complete database
name.
(sql-postgres-list-databases): New function returning a list of
available PostgreSQL databases.
(sql-get-login-ext): Don't require an exact match from completion.

* test/lisp/progmodes/sql-tests.el: New file with tests for sql.el.
---
 lisp/progmodes/sql.el            | 20 ++++++++++++----
 test/lisp/progmodes/sql-tests.el | 50 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)
 create mode 100644 test/lisp/progmodes/sql-tests.el

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index a11d456..a1710e5 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -1072,14 +1072,26 @@ sql-postgres-options
   :version "20.8"
   :group 'SQL)
 
-(defcustom sql-postgres-login-params `((user :default ,(user-login-name))
-                                       (database :default ,(user-login-name))
-                                       server)
+(defcustom sql-postgres-login-params
+  `((user :default ,(user-login-name))
+    (database :default ,(user-login-name)
+              :completion ,(completion-table-dynamic
+                            (lambda (_) (sql-postgres-list-databases))))
+    server)
   "List of login parameters needed to connect to Postgres."
   :type 'sql-login-params
   :version "24.1"
   :group 'SQL)
 
+(defun sql-postgres-list-databases ()
+  "Return a list of available PostgreSQL databases."
+  (when (executable-find "psql")
+    (let ((res '()))
+      (dolist (row (process-lines "psql" "-l"))
+        (when (string-match "^ \\([[:alnum:]-_]+\\) +|.*" row)
+          (push (substring row (match-beginning 1) (match-end 1)) res)))
+      res)))
+
 ;; Customization for Interbase
 
 (defcustom sql-interbase-program "isql"
@@ -2951,7 +2963,7 @@ sql-get-login-ext
                               (file-name-nondirectory f)))))))
 
       ((plist-member plist :completion)
-       (completing-read prompt-def (plist-get plist :completion) nil t
+       (completing-read prompt-def (plist-get plist :completion) nil nil
                         last-value history-var default))
 
       ((plist-get plist :number)
diff --git a/test/lisp/progmodes/sql-tests.el b/test/lisp/progmodes/sql-tests.el
new file mode 100644
index 0000000..9dc92c2
--- /dev/null
+++ b/test/lisp/progmodes/sql-tests.el
@@ -0,0 +1,50 @@
+;;; sql-tests.el --- Tests for sql.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2016  Simen Heggestøyl
+
+;; Author: Simen Heggestøyl <simenheg@gmail.com>
+;; Keywords:
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;
+
+;;; Code:
+
+(require 'cl-lib)
+(require 'ert)
+(require 'sql)
+
+(ert-deftest sql-tests-postgres-list-databases ()
+  "Test that output from `psql -l' is parsed correctly."
+  (cl-letf
+      (((symbol-function 'executable-find)
+        (lambda (_command) t))
+       ((symbol-function 'process-lines)
+        (lambda (_program &rest _args)
+          '("                                  List of databases                                  "
+            "   Name    |  Owner   | Encoding |   Collate   |    Ctype    |   Access privileges   "
+            "-----------+----------+----------+-------------+-------------+-----------------------"
+            " db-name-1 | foo-user | UTF8     | en_US.UTF-8 | en_US.UTF-8 |                       "
+            " db_name_2 | foo-user | UTF8     | en_US.UTF-8 | en_US.UTF-8 |                       "
+            "(2 rows)                                                                             "))))
+    (should (equal (sort (sql-postgres-list-databases) #'string<)
+                   '("db-name-1" "db_name_2")))))
+
+(provide 'sql-tests)
+;;; sql-tests.el ends here
-- 
2.9.3

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Simen Heggestøyl]

Ping.

Including some additional addresses in the CC field here in case I got
it wrong the first time.

-- Simen

On Sun, Oct 9, 2016 at 11:55 AM, Simen Heggestøyl <simenheg@gmail.com> 
wrote:
> Hi.
> 
> I recently started using sql.el with PostgreSQL, and thought it would
> be nice if `sql-postgres' would provide completion of available
> PostgreSQL databases.
> 
> Is something like the attached patch sufficient?
> 
> -- Simen
> 

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Michael Mauger]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

 Sorry for the delay. I did look at your fix and I need to test it but it looks reasonable. My concern is that it involves enough lines of code that I think we will require that you have a Copyright assignment on file. If you believe that you have one, I can verify that, if not we'll have to initiate the process (which unfortunately is manual and time consuming, but once done it is never needed again).

In the meantime, I'll review your patch and test further; I'll get back to you if I have any issues.
-- 
MICHAEL MAUGER // FSF Member // GNU Emacs sql.el maintainer // GNU Linux, GNU Emacs
 

    On Sunday, October 23, 2016 8:00 AM, Simen Heggestøyl <simenheg@gmail.com> wrote:
 
 

 Ping.

Including some additional addresses in the CC field here in case I got
it wrong the first time.

-- Simen

On Sun, Oct 9, 2016 at 11:55 AM, Simen Heggestøyl <simenheg@gmail.com> 
wrote:
> Hi.
> 
> I recently started using sql.el with PostgreSQL, and thought it would
> be nice if `sql-postgres' would provide completion of available
> PostgreSQL databases.
> 
> Is something like the attached patch sufficient?
> 
> -- Simen
> 


 
   

[-- Attachment #2: Type: text/html, Size: 3418 bytes --]

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Eli Zaretskii]

> Date: Sun, 23 Oct 2016 21:50:22 +0000 (UTC)
> From: Michael Mauger <michael@mauger.com>
> Cc: "alex@gnu.org" <alex@gnu.org>
> 
> Sorry for the delay. I did look at your fix and I need to test it but it looks reasonable. My concern is that it
> involves enough lines of code that I think we will require that you have a Copyright assignment on file. If you
> believe that you have one, I can verify that, if not we'll have to initiate the process (which unfortunately is
> manual and time consuming, but once done it is never needed again).

Simen's assignment is on file, so everything's good in that
department.

Thanks.

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Simen Heggestøyl]

On Sun, Oct 23, 2016 at 11:50 PM, Michael Mauger <michael@mauger.com> 
wrote:
> In the meantime, I'll review your patch and test further; I'll get 
> back to you if I have any issues.

Thanks, Michael. Did you get a chance to test it yet? If it saves you
time, I can install the patch once you're happy with it.

-- Simen

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Michael Mauger]

On Sunday, November 6, 2016 9:47 AM, Simen Heggestøyl <simenheg@gmail.com> wrote:
>On Sun, Oct 23, 2016 at 11:50 PM, Michael Mauger <michael@mauger.com> 
>wrote:
>> In the meantime, I'll review your patch and test further; I'll get 
>> back to you if I have any issues.
>
>Thanks, Michael. Did you get a chance to test it yet? If it saves you
>time, I can install the patch once you're happy with it.
>
>
>-- Simen


Several comments:
* Rather than hard-coding "psql", please use `sql-postgres-program' which can be customized to locate the psql executable.
* The "psql -l" command should also use the "-X,--no-psqlrc" option to avoid a configuration file overriding the field separator and changing the output format
* Rather than `(substring row (match-beginning 1) (match-end 1))' use `(match-string 1 row)'
* Return the nreverse of the result list so that the completion list is in the same order as psql lists them.
* `dolist' can specify the return value rather than having a separate expression after the loop. That is, (dolist (row (process-lines ...) (nreverse res)) ...)) is equivalent to (dolist (row (process-lines ...)) ...) (nreverse res)
* I'm concerned about the change to the `completing-read' call in `sql-get-login-ext'. Rather than `nil', I'd suggest `confirm' so that if the value isn't in the list, it must be confirmed.
* If the REQUIRE-MATCH parameter should really be something other than `t', then possibbly we should add another keyword :completion-required whose value would be used in the `completing-read' call (default to `t' to preserve current functionality).
* Thank you including the test module. I need to expand automated testing significantly and I appreciate your efforts to kickstart the effort and shame me to action :).


I greatly appreciate your submission but think we need to tighten up the code a bit before we commit it to the code base.

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

Thank you for your detailed feedback, Michael.

I agree with all of your points and have modified the patch accordingly,
except for one:

On Mon, Nov 7, 2016 at 5:03 AM, Michael Mauger <michael@mauger.com> 
wrote:
> * `dolist' can specify the return value rather than having a separate 
> expression after the loop. That is, (dolist (row (process-lines ...) 
> (nreverse res)) ...)) is equivalent to (dolist (row (process-lines 
> ...)) ...) (nreverse res)

I'm aware of it, but I tend to avoid using it, since I have many times
myself overlooked it when reading code that uses it. If it's OK with you
I'll leave it like it is, but if you insist I can change it.

> * I'm concerned about the change to the `completing-read' call in 
> `sql-get-login-ext'. Rather than `nil', I'd suggest `confirm' so that 
> if the value isn't in the list, it must be confirmed.
> * If the REQUIRE-MATCH parameter should really be something other 
> than `t', then possibbly we should add another keyword 
> :completion-required whose value would be used in the 
> `completing-read' call (default to `t' to preserve current 
> functionality).

It's not important to me. I made the change because I thought it could
be frustrating for the user if `sql-postgres-list-databases' doesn't
work as it should, but I realize now that changing it probably deserves
a discussion and a patch on its own.

-- Simen

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Complete-the-name-of-PostgreSQL-databases.patch --]
[-- Type: text/x-patch, Size: 3867 bytes --]

From b26312bc9000f8ec39a525eefad5f0c93c767b86 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Thu, 6 Oct 2016 20:42:53 +0200
Subject: [PATCH] Complete the name of PostgreSQL databases

* lisp/progmodes/sql.el (sql-postgres-login-params): Complete database
name.
(sql-postgres-list-databases): New function returning a list of
available PostgreSQL databases.

* test/lisp/progmodes/sql-tests.el: New file with tests for sql.el.
---
 lisp/progmodes/sql.el            | 18 ++++++++++++---
 test/lisp/progmodes/sql-tests.el | 47 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 3 deletions(-)
 create mode 100644 test/lisp/progmodes/sql-tests.el

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index a11d456..4d0bed7 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -1072,14 +1072,26 @@ sql-postgres-options
   :version "20.8"
   :group 'SQL)
 
-(defcustom sql-postgres-login-params `((user :default ,(user-login-name))
-                                       (database :default ,(user-login-name))
-                                       server)
+(defcustom sql-postgres-login-params
+  `((user :default ,(user-login-name))
+    (database :default ,(user-login-name)
+              :completion ,(completion-table-dynamic
+                            (lambda (_) (sql-postgres-list-databases))))
+    server)
   "List of login parameters needed to connect to Postgres."
   :type 'sql-login-params
   :version "24.1"
   :group 'SQL)
 
+(defun sql-postgres-list-databases ()
+  "Return a list of available PostgreSQL databases."
+  (when (executable-find sql-postgres-program)
+    (let ((res '()))
+      (dolist (row (process-lines sql-postgres-program "-ltX"))
+        (when (string-match "^ \\([[:alnum:]-_]+\\) +|.*" row)
+          (push (match-string 1 row) res)))
+      (nreverse res))))
+
 ;; Customization for Interbase
 
 (defcustom sql-interbase-program "isql"
diff --git a/test/lisp/progmodes/sql-tests.el b/test/lisp/progmodes/sql-tests.el
new file mode 100644
index 0000000..e05247a
--- /dev/null
+++ b/test/lisp/progmodes/sql-tests.el
@@ -0,0 +1,47 @@
+;;; sql-tests.el --- Tests for sql.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; Author: Simen Heggestøyl <simenheg@gmail.com>
+;; Keywords:
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;
+
+;;; Code:
+
+(require 'cl-lib)
+(require 'ert)
+(require 'sql)
+
+(ert-deftest sql-tests-postgres-list-databases ()
+  "Test that output from `psql -ltX' is parsed correctly."
+  (cl-letf
+      (((symbol-function 'executable-find)
+        (lambda (_command) t))
+       ((symbol-function 'process-lines)
+        (lambda (_program &rest _args)
+          '(" db-name-1 | foo-user | UTF8     | en_US.UTF-8 | en_US.UTF-8 | "
+            " db_name_2 | foo-user | UTF8     | en_US.UTF-8 | en_US.UTF-8 | "
+            ""))))
+    (should (equal (sql-postgres-list-databases)
+                   '("db-name-1" "db_name_2")))))
+
+(provide 'sql-tests)
+;;; sql-tests.el ends here
-- 
2.10.2

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Michael Mauger]

> On Saturday, November 12, 2016 6:12 AM, Simen Heggestøyl <simenheg@gmail.com> wrote:

> > Thank you for your detailed feedback, Michael.
> 
> I agree with all of your points and have modified the patch accordingly,
> except for one:
> 
> On Mon, Nov 7, 2016 at 5:03 AM, Michael Mauger <michael@mauger.com> 
> wrote:
>>  * `dolist' can specify the return value rather than having a separate 
>>  expression after the loop. That is, (dolist (row (process-lines ...) 
>>  (nreverse res)) ...)) is equivalent to (dolist (row (process-lines 
>>  ...)) ...) (nreverse res)
> 
> I'm aware of it, but I tend to avoid using it, since I have many times
> myself overlooked it when reading code that uses it. If it's OK with you
> I'll leave it like it is, but if you insist I can change it.
> 
>>  * I'm concerned about the change to the `completing-read' call in 
>>  `sql-get-login-ext'. Rather than `nil', I'd suggest 
> `confirm' so that 
>>  if the value isn't in the list, it must be confirmed.
>>  * If the REQUIRE-MATCH parameter should really be something other 
>>  than `t', then possibbly we should add another keyword 
>>  :completion-required whose value would be used in the 
>>  `completing-read' call (default to `t' to preserve current 
>>  functionality).
> 
> It's not important to me. I made the change because I thought it could
> be frustrating for the user if `sql-postgres-list-databases' doesn't
> work as it should, but I realize now that changing it probably deserves
> a discussion and a patch on its own.
> 
> 
> -- Simen

> 


Looks good. Go ahead and commit this. 


Thank you, I appreciate the contribution. 


I'll take a look at making the completion REQUIRE-MATCH change separately; I do think that control of completion behavior is needed here but it impacts more than just database selection for Postgres. The advantage of adding a keyword to the `sql-product-alist' is that the user can control the behavior to meet their preferences.

-- 
MICHAEL MAUGER // FSF Member // GNU Emacs sql-mode maintainer // GNU Linux, GNU Emacs

bug#24646: [PATCH] Complete the name of PostgreSQL databases

[Simen Heggestøyl]

Thanks, Michael. I've commited it to master.

-- Simen