[DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses
Greetings
We have following syntax:
ALTER THING name [ NO ] DEPENDS ON EXTENSION name
for the following THINGs:
- ALTER TRIGGER
- ALTER FUNCTION
- ALTER PROCEDURE
- ALTER ROUTINE
- ALTER MATERIALIZED VIEW
- ALTER INDEX
In the documentation, the "[ NO ]" option is listed in the synopsis for
ALTER TRIGGER and ALTER FUNCTION, but not the others.
Trivial patch attached.
Will add to next CF.
Regards
Ian Barwick
--
EnterpriseDB: https://www.enterprisedb.com
Attachments:
doc-alter-thing-no-depends-on.v1.patchtext/x-patch; charset=US-ASCII; name=doc-alter-thing-no-depends-on.v1.patchDownload+4-4
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
2021年2月13日(土) 11:52 Michael Paquier <michael@paquier.xyz>:
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
--
EnterpriseDB: https://www.enterprisedb.com
2021年2月13日(土) 11:52 Michael Paquier <michael@paquier.xyz>:
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.
Thanks! (Apologies for the preceding blank mail).
It is worth noting that tab-complete.c does a bad
job in completing those clauses.
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.
Regards
Ian Barwick
--
EnterpriseDB: https://www.enterprisedb.com
Attachments:
psql-no-depends-tab-completion.v1.difftext/x-patch; charset=US-ASCII; name=psql-no-depends-tab-completion.v1.diffDownload+16-9
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
2021年2月16日(火) 10:20 Michael Paquier <michael@paquier.xyz>:
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.
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.
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?).
Regards
Ian Barwick
--
EnterpriseDB: https://www.enterprisedb.com
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:
/messages/by-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