Improve tab completion for ALTER FUNCTION/PROCEDURE/ROUTINE

Hi,

Tab completion for ALTER FUNCTION/PROCEDURE/ROUTINE action was
missing, this patch adds the tab completion for the same.

Regards,
Vignesh

On Fri, Oct 28, 2022 at 12:08 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> Hi,
>
> Tab completion for ALTER FUNCTION/PROCEDURE/ROUTINE action was
> missing, this patch adds the tab completion for the same.
>
> Regards,
> Vignesh

Hi,
I applied your patch and did some tests.
Is it okay not to consider SET and RESET commands? (e.g ALTER FUNCTION)

---
Regards,
DongWook Lee.

On Fri, 28 Oct 2022 at 08:02, Dong Wook Lee <sh95119(at)gmail(dot)com> wrote:
>
> On Fri, Oct 28, 2022 at 12:08 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > Tab completion for ALTER FUNCTION/PROCEDURE/ROUTINE action was
> > missing, this patch adds the tab completion for the same.
> >
> > Regards,
> > Vignesh
>
> Hi,
> I applied your patch and did some tests.
> Is it okay not to consider SET and RESET commands? (e.g ALTER FUNCTION)

Those also should be handled, attached v2 version includes the changes
for the same.

Regards,
Vignesh

On Fri, Oct 28, 2022 at 05:34:37PM +0530, vignesh C wrote:
> Those also should be handled, attached v2 version includes the changes
> for the same.

The basic options supported by PROCEDURE are a subset of ROUTINE with a
difference of COST, IMMUTABLE, [NOT] LEAKPROOF, ROWS, STABLE
and VOLATILE.

The basic options supported by ROUTINE are a subset of FUNCTION with a
difference of { CALLED | RETURNS NULL } ON NULL INPUT, STRICT and
SUPPORT. Is it worth refactoring a bit with common lists?

+ "RESET", "RETURNS NULL ON NULL INPUT ", "ROWS",
Extra space after INPUT here, that's easy to miss.
--
Michael

On Tue, 22 Nov 2022 at 05:59, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Oct 28, 2022 at 05:34:37PM +0530, vignesh C wrote:
> > Those also should be handled, attached v2 version includes the changes
> > for the same.
>
> The basic options supported by PROCEDURE are a subset of ROUTINE with a
> difference of COST, IMMUTABLE, [NOT] LEAKPROOF, ROWS, STABLE
> and VOLATILE.
>
> The basic options supported by ROUTINE are a subset of FUNCTION with a
> difference of { CALLED | RETURNS NULL } ON NULL INPUT, STRICT and
> SUPPORT. Is it worth refactoring a bit with common lists?

Modified

> + "RESET", "RETURNS NULL ON NULL INPUT ", "ROWS",
> Extra space after INPUT here, that's easy to miss.

Good catch, the attached v3 patch has the changes for the same.

Regards,
Vignesh

Hi Vignesh,

Looks like the patch needs a rebase.

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"

Best,
--
Melih Mutlu
Microsoft

On Tue, 6 Dec 2022 at 20:42, Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
>
> Hi Vignesh,
>
> Looks like the patch needs a rebase.

Rebased

> 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.
The attached v4 patch is rebased on top of HEAD.

Regards,
Vignesh

Hi,

vignesh C <vignesh21(at)gmail(dot)com>, 6 Ara 2022 Sal, 22:12 tarihinde şunu yazdı:

> 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.
>

Okay, makes sense.

I tested the patch and it worked for me.

Best,
--
Melih Mutlu
Microsoft

On Tue, 6 Dec 2022 at 19:12, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 6 Dec 2022 at 20:42, Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)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

On Thu, 5 Jan 2023 at 18:22, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Tue, 6 Dec 2022 at 19:12, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Tue, 6 Dec 2022 at 20:42, Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)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.

The changes look good to me.

Regards,
Vignesh

On Fri, 6 Jan 2023 at 02:38, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 5 Jan 2023 at 18:22, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> >
> > That leads to the attached, which barring objections, I'll push shortly.
>
> The changes look good to me.
>

Pushed.

Regards,
Dean

On Thu, 5 Jan 2023 at 12:52, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> 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.
>

OK, I've pushed and back-patched a fix for that issue too.

Regards,
Dean

On Fri, 6 Jan 2023 at 15:33, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Fri, 6 Jan 2023 at 02:38, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Thu, 5 Jan 2023 at 18:22, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> > >
> > > That leads to the attached, which barring objections, I'll push shortly.
> >
> > The changes look good to me.
> >
>
> Pushed.

Thanks for pushing this.

Regards,
Vignesh