[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#24646: [PATCH] Complete the name of PostgreSQL databases
From: |
Simen Heggestøyl |
Subject: |
bug#24646: [PATCH] Complete the name of PostgreSQL databases |
Date: |
Sat, 12 Nov 2016 12:11:42 +0100 |
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
0001-Complete-the-name-of-PostgreSQL-databases.patch
Description: Text Data