Thread: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

On Fri, Feb 12, 2021 at 10:32:14AM +0900, Ian Lawrence Barwick wrote:
> In the documentation, the "[ NO ]" option is listed in the synopsis for
> ALTER TRIGGER and ALTER FUNCTION, but not the others.
> Trivial patch attached.

There are two flavors to cover for 6 commands per gram.y, and you are
covering all of them.  So this looks good to me.  I'll apply and
backpatch in a bit.  It is worth noting that tab-complete.c does a bad
job in completing those clauses.
--
Michael

On Mon, Feb 15, 2021 at 03:57:04PM +0900, Ian Lawrence Barwick wrote:
> Indeed it does. Not the most exciting of use cases, though I imagine it
> might come in handy for anyone developing an extension, and the
> existing implementation is inconsistent (in place for ALTER INDEX,
> and partially for ALTER MATERIALIZED VIEW, but not the others).
> Patch suggestion attached.

Thanks.

-   else if (Matches("ALTER", "INDEX", MatchAny, "NO", "DEPENDS"))
-       COMPLETE_WITH("ON EXTENSION");
-   else if (Matches("ALTER", "INDEX", MatchAny, "DEPENDS"))
-       COMPLETE_WITH("ON EXTENSION");
The part, if removed, means that typing "alter index my_index no " is
not able to complete with "DEPENDS ON EXTENSION" anymore.  So it seems
to me that ALTER INDEX got that right, and that the other commands had
better do the same.
--
Michael

On Tue, Feb 16, 2021 at 11:18:47AM +0900, Ian Lawrence Barwick wrote:
> Hmm, with the current implementation "alter index my_index no <TAB>"
> doesn't work
> anyway; you'd need to add this before the above lines:
>
> +       else if (Matches("ALTER", "INDEX", MatchAny, "NO"))
> +               COMPLETE_WITH("DEPENDS");
>
> so AFAICT the patch doesn't change that behaviour. It does mean "alter index
> my_index no depends <TAB>" no longer completes to "ON EXTENSION", but if
> you've
> typed one of "NO" or "DEPENDS" in that context, "ON EXTENSION" is the only
> completion so I'm not sure what's gained by forcing the user to hit TAB
> twice.

You are right.  It looks like I have tested without a whitespace after
the "NO".  With a whitespace it does not work, so that looks like a
complication for little gain.  Another problem with the code on HEAD
is that you would not complete properly "NO DEPENDS ON", so that feels
half-completed.

> There are quite a few tab completions consisting of more than one word
> (e.g. "MATERIALIZED VIEW", "FORCE ROW LEVEL SECURITY") where tab completion
> is
> ineffective after the first word followed by a space, e.g. "alter
> materialized
> <TAB>" doesn't result in any expansion either. I suppose we could go
> through all
> those and handle each word individually, but presumably there's a reason why
> that hasn't been done already (maybe no-one has complained?).

Because that's just extra maintenance as most people will just
complete after typing the first set of characters?  This part got
discussed as of 1e324cb:
https://www.postgresql.org/message-id/CALtqXTcogrFEVP9uou5vFtnGsn+vHZUu9+9a0inarfYVOHScYQ@mail.gmail.com

Anyway, after sleeping on it, I have just applied your original patch
as that's simpler, and will cover the cases people would care for.
--
Michael