On Tue, 6 Dec 2022 at 19:12, vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 6 Dec 2022 at 20:42, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> >
> > Also one little suggestion:
> >
> >> + if (ends_with(prev_wd, ')'))
> >> + COMPLETE_WITH(Alter_routine_options, "CALLED ON NULL INPUT",
> >> + "RETURNS NULL ON NULL INPUT", "STRICT", "SUPPORT");
> >
> > What do you think about gathering FUNCTION options as you did with ROUTINE options.
> > Something like the following would seem nicer, I think.
> >
> >> #define Alter_function_options \
> >> Alter_routine_options, "CALLED ON NULL INPUT", \
> >> "RETURNS NULL ON NULL INPUT", "STRICT", "SUPPORT"
>
> I did not make it as a macro for alter function options as it is used
> only in one place whereas the others were required in more than one
> place.
My feeling is that having this macro somewhat improves readability and
consistency between the 3 cases, so I think it's worth it, even if
it's only used once.
I think it slightly improves readability to keep all the arguments to
Matches() on one line, and that seems to be the style elsewhere, even
if that makes the line longer than 80 characters.
Also in the interests of readability, I think it's slightly easier to
follow if the "ALTER PROCEDURE <name> (...)" and "ALTER ROUTINE <name>
(...)" cases are made to immediately follow the "ALTER FUNCTION <name>
(...)" case, with the longer/more complex cases following on from
that.
That leads to the attached, which barring objections, I'll push shortly.
While playing around with this, I noticed that the "... SET SCHEMA"
case offers "FROM CURRENT" and "TO" as completions, which is
incorrect. It should really offer to complete with a list of schemas.
However, since that's a pre-existing bug in a different region of the
code, I think it's best addressed in a separate patch, which probably
ought to be back-patched.
Regards,
Dean
