On Fri, 9 Dec 2022 at 16:01, Christoph Heiss <christoph@c8h4.io> wrote:
>
> Thanks for the review!
>
> On 12/8/22 12:19, Melih Mutlu wrote:
> > Hi Christoph,
> >
> > I just took a quick look at your patch.
> > Some suggestions:
> >
> > + else if (Matches("ALTER", "VIEW", MatchAny, "SET", "("))
> > + COMPLETE_WITH_LIST(view_optional_parameters);
> > + /* ALTER VIEW xxx RESET ( yyy , ... ) */
> > + else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "("))
> > + COMPLETE_WITH_LIST(view_optional_parameters);
> >
> >
> > What about combining these two cases into one like Matches("ALTER",
> > "VIEW", MatchAny, "SET|RESET", "(") ?
> Good thinking, incorporated that into v2.
> While at it, I also added completion for the values of the SET
> parameters, since that is useful as well.
>
> >
> > /* ALTER VIEW <name> */
> > else if (Matches("ALTER", "VIEW", MatchAny))
> > COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
> > "SET SCHEMA");
> >
> >
> > Also seems like SET and RESET don't get auto-completed for "ALTER VIEW
> > <name>".
> > I think it would be nice to include those missing words.
> That is already contained in the patch:
>
> @@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int end)
> /* ALTER VIEW <name> */
> else if (Matches("ALTER", "VIEW", MatchAny))
> COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
> - "SET SCHEMA");
> + "SET SCHEMA", "SET (", "RESET (");
One suggestion:
Tab completion for "alter view v1 set (check_option =" is handled to
complete with CASCADED and LOCAL but the same is not handled for
create view: "create view viewname with ( check_option ="
+ else if (Matches("ALTER", "VIEW", MatchAny, "SET", "(",
"check_option", "="))
+ COMPLETE_WITH("local", "cascaded");
I felt we should keep the handling consistent for both create view and
alter view.
Regards,
Vignesh