mprove tab completion for ALTER EXTENSION ADD/DROP

------- Original Message -------
On Sunday, November 27th, 2022 at 10:24, vignesh C <vignesh21@gmail.com> wrote:

Hi,

Tab completion for ALTER EXTENSION ADD and DROP was missing, this
patch adds the tab completion for the same.

Regards,
Vignesh

Hi Vignesh

I've tested your patched on current master and seems to be working properly.

I'm starting reviewing some patches here, let's see what more experience hackers
has to say about this, but as far I can tell is that is working as expected.

--
Matheus Alcantara

On Sat, Dec 03, 2022 at 05:34:57PM +0000, Matheus Alcantara wrote:

I've tested your patched on current master and seems to be working properly.

I'm starting reviewing some patches here, let's see what more experience hackers
has to say about this, but as far I can tell is that is working as expected.

+       /* ALTER EXTENSION <name> ADD|DROP */
+       else if (Matches("ALTER", "EXTENSION", MatchAny, "ADD|DROP"))
+               COMPLETE_WITH("ACCESS METHOD", "AGGREGATE", "CAST", "COLLATION",
+                                         "CONVERSION", "DOMAIN", "EVENT TRIGGER", "FOREIGN",
+                                         "FUNCTION", "MATERIALIZED VIEW", "OPERATOR",
+                                         "PROCEDURAL LANGUAGE", "PROCEDURE", "LANGUAGE",
+                                         "ROUTINE", "SCHEMA", "SEQUENCE", "SERVER", "TABLE",
+                                         "TEXT SEARCH", "TRANSFORM FOR", "TYPE", "VIEW");
+
+       /* ALTER EXTENSION <name> ADD|DROP FOREIGN*/
+       else if (Matches("ALTER", "EXTENSION", MatchAny, "ADD|DROP", "FOREIGN"))
+               COMPLETE_WITH("DATA WRAPPER", "TABLE");

The DROP could be matched with the objects that are actually part of
the so-said extension?
--
Michael

On Mon, 5 Dec 2022 at 06:53, Michael Paquier <michael@paquier.xyz> wrote:

On Sat, Dec 03, 2022 at 05:34:57PM +0000, Matheus Alcantara wrote:

I've tested your patched on current master and seems to be working properly.

I'm starting reviewing some patches here, let's see what more experience hackers
has to say about this, but as far I can tell is that is working as expected.

+       /* ALTER EXTENSION <name> ADD|DROP */
+       else if (Matches("ALTER", "EXTENSION", MatchAny, "ADD|DROP"))
+               COMPLETE_WITH("ACCESS METHOD", "AGGREGATE", "CAST", "COLLATION",
+                                         "CONVERSION", "DOMAIN", "EVENT TRIGGER", "FOREIGN",
+                                         "FUNCTION", "MATERIALIZED VIEW", "OPERATOR",
+                                         "PROCEDURAL LANGUAGE", "PROCEDURE", "LANGUAGE",
+                                         "ROUTINE", "SCHEMA", "SEQUENCE", "SERVER", "TABLE",
+                                         "TEXT SEARCH", "TRANSFORM FOR", "TYPE", "VIEW");
+
+       /* ALTER EXTENSION <name> ADD|DROP FOREIGN*/
+       else if (Matches("ALTER", "EXTENSION", MatchAny, "ADD|DROP", "FOREIGN"))
+               COMPLETE_WITH("DATA WRAPPER", "TABLE");

The DROP could be matched with the objects that are actually part of
the so-said extension?

The modified v2 version has the changes to handle the same. Sorry for
the delay as I was working on another project.

Regards,
Vignesh

At Mon, 2 Jan 2023 13:19:50 +0530, vignesh C <vignesh21@gmail.com> wrote in

On Mon, 5 Dec 2022 at 06:53, Michael Paquier <michael@paquier.xyz> wrote:

The DROP could be matched with the objects that are actually part of
the so-said extension?

The modified v2 version has the changes to handle the same. Sorry for
the delay as I was working on another project.

It suggests the *kinds* of objects that are part of the extension, but
lists the objects of that kind regardless of dependency. I read
Michael suggested (and I agree) to restrict the objects (not kinds) to
actually be a part of the extension. (And not for object kinds.)

However I'm not sure it is useful to restrict object kinds since the
operator already knows what to drop, if you still want to do that, the
use of completion_dont_quote looks ugly since the function
(requote_identifier) is assuming an identifier as input. I didn't
looked closer but maybe we need another way to do that.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Jan 11, 2023 at 12:10:33PM +0900, Kyotaro Horiguchi wrote:

It suggests the *kinds* of objects that are part of the extension, but
lists the objects of that kind regardless of dependency. I read
Michael suggested (and I agree) to restrict the objects (not kinds) to
actually be a part of the extension. (And not for object kinds.)

Yeah, that's what I meant. Now, if Vignesh does not want to extend
that, that's fine for me as well at the end on second thought, as this
involves much more code for each DROP path depending on the object
type involved.

Adding the object names after DROP/ADD is useful on its own, and we
already have some completion once the object type is specified, so
simpler is perhaps just better here.
--
Michael

On Wed, 11 Jan 2023 at 12:19, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jan 11, 2023 at 12:10:33PM +0900, Kyotaro Horiguchi wrote:

It suggests the *kinds* of objects that are part of the extension, but
lists the objects of that kind regardless of dependency. I read
Michael suggested (and I agree) to restrict the objects (not kinds) to
actually be a part of the extension. (And not for object kinds.)

Yeah, that's what I meant. Now, if Vignesh does not want to extend
that, that's fine for me as well at the end on second thought, as this
involves much more code for each DROP path depending on the object
type involved.

Adding the object names after DROP/ADD is useful on its own, and we
already have some completion once the object type is specified, so
simpler is perhaps just better here.

I too felt keeping it simpler is better. How about using the simple
first version of patch itself?

Regards,
Vignesh

On Wed, Jan 11, 2023 at 10:29:25PM +0530, vignesh C wrote:

I too felt keeping it simpler is better. How about using the simple
first version of patch itself?

Okay, I have just done that, then, after checking that all the object
types were covered (28). Note that PROCEDURAL LANGUAGE has been
removed for simplicity.
--
Michael

On Thu, 12 Jan 2023 at 05:22, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jan 11, 2023 at 10:29:25PM +0530, vignesh C wrote:

I too felt keeping it simpler is better. How about using the simple
first version of patch itself?

Okay, I have just done that, then, after checking that all the object
types were covered (28). Note that PROCEDURAL LANGUAGE has been
removed for simplicity.

Thanks for pushing this.

Regards,
Vignesh