Обсуждение: Several problems in tab-completions for SET/RESET

Hi,

I found that the following tab-completions for SET/RESET which
worked properly before doesn't work properly now in the master.

1. ALTER SYSTEM SET|RESET <tab> lists nothing.
2. ALTER DATABASE xxx SET <tab> lists nothing.
3. ALTER DATABASE xxx SET yyy <tab> lists nothing.
4. ALTER DATABASE xxx SET datestyle TO <tab> lists nothing.

Attached patch fixes those problems.

Regards,

--
Fujii Masao

On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> I found that the following tab-completions for SET/RESET which
> worked properly before doesn't work properly now in the master.
>
> 1. ALTER SYSTEM SET|RESET <tab> lists nothing.
> 2. ALTER DATABASE xxx SET <tab> lists nothing.
> 3. ALTER DATABASE xxx SET yyy <tab> lists nothing.
> 4. ALTER DATABASE xxx SET datestyle TO <tab> lists nothing.
>
> Attached patch fixes those problems.

-       else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
+       else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
Good catch.

-       else if (Matches2("SET", MatchAny))
+       else if (TailMatches2("SET", MatchAny) &&
+                        !TailMatches4("UPDATE|DOMAIN", MatchAny,
MatchAny, MatchAny) &&
+                        !TailMatches1("TABLESPACE|SCHEMA") &&
+                        !ends_with(prev_wd, ')') &&
+                        !ends_with(prev_wd, '='))               COMPLETE_WITH_CONST("TO");
This gets... unreadable.

In order to maximize the amount of Matches() used, wouldn't it be
better to complete a bit more the list of completions directly in
ALTER DATABASE? This would make the code more readable.
-- 
Michael

On Thu, Jan 28, 2016 at 10:15 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> I found that the following tab-completions for SET/RESET which
>> worked properly before doesn't work properly now in the master.
>>
>> 1. ALTER SYSTEM SET|RESET <tab> lists nothing.
>> 2. ALTER DATABASE xxx SET <tab> lists nothing.
>> 3. ALTER DATABASE xxx SET yyy <tab> lists nothing.
>> 4. ALTER DATABASE xxx SET datestyle TO <tab> lists nothing.
>>
>> Attached patch fixes those problems.
>
> -       else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
> +       else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
> Good catch.
>
> -       else if (Matches2("SET", MatchAny))
> +       else if (TailMatches2("SET", MatchAny) &&
> +                        !TailMatches4("UPDATE|DOMAIN", MatchAny,
> MatchAny, MatchAny) &&
> +                        !TailMatches1("TABLESPACE|SCHEMA") &&
> +                        !ends_with(prev_wd, ')') &&
> +                        !ends_with(prev_wd, '='))
>                 COMPLETE_WITH_CONST("TO");

This change breaks tab completion for ALTER TABLE ... SET
[WITH/LOGGED/UNLOGGED].

It think it should be
> +       else if (Matches2("SET", MatchAny) &&

Related to it, I found tab completion for ALTER TABLE .. SET WITH,
which doesn't working well.
Patch is attached.

Regards,

--
Masahiko Sawada

On Thu, Jan 28, 2016 at 10:15 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> I found that the following tab-completions for SET/RESET which
>> worked properly before doesn't work properly now in the master.
>>
>> 1. ALTER SYSTEM SET|RESET <tab> lists nothing.
>> 2. ALTER DATABASE xxx SET <tab> lists nothing.
>> 3. ALTER DATABASE xxx SET yyy <tab> lists nothing.
>> 4. ALTER DATABASE xxx SET datestyle TO <tab> lists nothing.
>>
>> Attached patch fixes those problems.
>
> -       else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
> +       else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
> Good catch.
>
> -       else if (Matches2("SET", MatchAny))
> +       else if (TailMatches2("SET", MatchAny) &&
> +                        !TailMatches4("UPDATE|DOMAIN", MatchAny,
> MatchAny, MatchAny) &&
> +                        !TailMatches1("TABLESPACE|SCHEMA") &&
> +                        !ends_with(prev_wd, ')') &&
> +                        !ends_with(prev_wd, '='))
>                 COMPLETE_WITH_CONST("TO");
> This gets... unreadable.
>
> In order to maximize the amount of Matches() used, wouldn't it be
> better to complete a bit more the list of completions directly in
> ALTER DATABASE? This would make the code more readable.

Thanks for the review!

I removed the above and added the following for that case.

+    /* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET <name> */
+    else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
+             TailMatches2("SET", MatchAny))
+        COMPLETE_WITH_LIST2("FROM CURRENT", "TO");

Attached is the updated version of the patch.
I also added the change that Sawada suggested, to the patch.

Regards,

--
Fujii Masao

On Thu, Jan 28, 2016 at 10:50 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Thu, Jan 28, 2016 at 10:15 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> I found that the following tab-completions for SET/RESET which
>>> worked properly before doesn't work properly now in the master.
>>>
>>> 1. ALTER SYSTEM SET|RESET <tab> lists nothing.
>>> 2. ALTER DATABASE xxx SET <tab> lists nothing.
>>> 3. ALTER DATABASE xxx SET yyy <tab> lists nothing.
>>> 4. ALTER DATABASE xxx SET datestyle TO <tab> lists nothing.
>>>
>>> Attached patch fixes those problems.
>>
>> -       else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
>> +       else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
>> Good catch.
>>
>> -       else if (Matches2("SET", MatchAny))
>> +       else if (TailMatches2("SET", MatchAny) &&
>> +                        !TailMatches4("UPDATE|DOMAIN", MatchAny,
>> MatchAny, MatchAny) &&
>> +                        !TailMatches1("TABLESPACE|SCHEMA") &&
>> +                        !ends_with(prev_wd, ')') &&
>> +                        !ends_with(prev_wd, '='))
>>                 COMPLETE_WITH_CONST("TO");
>
> This change breaks tab completion for ALTER TABLE ... SET
> [WITH/LOGGED/UNLOGGED].

Thanks for the review!
Fixed.

> It think it should be
>> +       else if (Matches2("SET", MatchAny) &&
>
> Related to it, I found tab completion for ALTER TABLE .. SET WITH,
> which doesn't working well.
> Patch is attached.

Please see the latest patch that I posted upthread.
I included your patch in that patch.

Regards,

-- 
Fujii Masao

On Fri, Jan 29, 2016 at 11:53 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> I removed the above and added the following for that case.
>
> +    /* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET <name> */
> +    else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
> +             TailMatches2("SET", MatchAny))
> +        COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
>
> Attached is the updated version of the patch.

"ALTER FUNCTION foo(bar)" suggests OWNER TO, RENAME TO and SET SCHEMA.
I think that we had better suggesting SET instead of SET SCHEMA, and
add SCHEMA in the list of things suggested by SET.

"ALTER DATABASE foodb SET foo_param" should suggest TO/= but that's
not the case. After adding TO/= manually, a list of values is
suggested though. Same problem with ALTER ROLE and ALTER FUNCTION.

> I also added the change that Sawada suggested, to the patch.

OK.
-- 
Michael

On Fri, Jan 29, 2016 at 1:02 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Jan 29, 2016 at 11:53 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> I removed the above and added the following for that case.
>>
>> +    /* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET <name> */
>> +    else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
>> +             TailMatches2("SET", MatchAny))
>> +        COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
>>
>> Attached is the updated version of the patch.

Thanks for the review!

> "ALTER FUNCTION foo(bar)" suggests OWNER TO, RENAME TO and SET SCHEMA.
> I think that we had better suggesting SET instead of SET SCHEMA, and
> add SCHEMA in the list of things suggested by SET.

Maybe, and it should suggest other keywords like RESET. That's it's better to
overhaul the tab-completion of ALTER FUNCTION. But that's not the task of
this patch. IMO it's better to fix that as a separate patch.

> "ALTER DATABASE foodb SET foo_param" should suggest TO/= but that's
> not the case. After adding TO/= manually, a list of values is
> suggested though. Same problem with ALTER ROLE and ALTER FUNCTION.

Fixed. Attached is the updated version of the patch.

Regards,

--
Fujii Masao

On Mon, Feb 1, 2016 at 1:21 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, Jan 29, 2016 at 1:02 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Jan 29, 2016 at 11:53 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> I removed the above and added the following for that case.
>>>
>>> +    /* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET <name> */
>>> +    else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
>>> +             TailMatches2("SET", MatchAny))
>>> +        COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
>>>
>>> Attached is the updated version of the patch.
>
> Thanks for the review!
>
>> "ALTER FUNCTION foo(bar)" suggests OWNER TO, RENAME TO and SET SCHEMA.
>> I think that we had better suggesting SET instead of SET SCHEMA, and
>> add SCHEMA in the list of things suggested by SET.
>
> Maybe, and it should suggest other keywords like RESET. That's it's better to
> overhaul the tab-completion of ALTER FUNCTION. But that's not the task of
> this patch. IMO it's better to fix that as a separate patch.

Er, OK... I thought that both problems seem rather linked per the
$subject but I can send an extra patch on this thread if necessary.
Never mind.

>> "ALTER DATABASE foodb SET foo_param" should suggest TO/= but that's
>> not the case. After adding TO/= manually, a list of values is
>> suggested though. Same problem with ALTER ROLE and ALTER FUNCTION.
>
> Fixed. Attached is the updated version of the patch.

+       /* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET <name> */
+       else if (HeadMatches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
+                        TailMatches2("SET", MatchAny))
+               COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
Small thing: adding "=" to the list of things that can be completed?

Except that the rest looks fine to me.
-- 
Michael

On Mon, Feb 1, 2016 at 3:14 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Feb 1, 2016 at 1:21 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Fri, Jan 29, 2016 at 1:02 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> On Fri, Jan 29, 2016 at 11:53 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> I removed the above and added the following for that case.
>>>>
>>>> +    /* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET <name> */
>>>> +    else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
>>>> +             TailMatches2("SET", MatchAny))
>>>> +        COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
>>>>
>>>> Attached is the updated version of the patch.
>>
>> Thanks for the review!
>>
>>> "ALTER FUNCTION foo(bar)" suggests OWNER TO, RENAME TO and SET SCHEMA.
>>> I think that we had better suggesting SET instead of SET SCHEMA, and
>>> add SCHEMA in the list of things suggested by SET.
>>
>> Maybe, and it should suggest other keywords like RESET. That's it's better to
>> overhaul the tab-completion of ALTER FUNCTION. But that's not the task of
>> this patch. IMO it's better to fix that as a separate patch.
>
> Er, OK... I thought that both problems seem rather linked per the
> $subject but I can send an extra patch on this thread if necessary.
> Never mind.
>
>>> "ALTER DATABASE foodb SET foo_param" should suggest TO/= but that's
>>> not the case. After adding TO/= manually, a list of values is
>>> suggested though. Same problem with ALTER ROLE and ALTER FUNCTION.
>>
>> Fixed. Attached is the updated version of the patch.
>
> +       /* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET <name> */
> +       else if (HeadMatches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
> +                        TailMatches2("SET", MatchAny))
> +               COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
> Small thing: adding "=" to the list of things that can be completed?

If we do that, we also should change the tab-completion for SET command
so that "=" is suggested. But I'm afraid that which might decrease that
tab-completion.

Imagine the case of "SET work_mem <tab>". If "TO" and "=" are suggested,
we need to type either "T" or "=" and then <tab> to input the setting value.
Otherwise, "SET work_mem <tab>" suggests only "TO" and we can input
the setting value by just typing <tab> again.
This extra step is very small, but SET command is usually used very often,
so I'd like to avoid such extra step.

Regards,

-- 
Fujii Masao

On Mon, Feb 1, 2016 at 9:15 PM, Fujii Masao wrote:
> If we do that, we also should change the tab-completion for SET command
> so that "=" is suggested. But I'm afraid that which might decrease that
> tab-completion.
>
> Imagine the case of "SET work_mem <tab>". If "TO" and "=" are suggested,
> we need to type either "T" or "=" and then <tab> to input the setting value.
> Otherwise, "SET work_mem <tab>" suggests only "TO" and we can input
> the setting value by just typing <tab> again.
> This extra step is very small, but SET command is usually used very often,
> so I'd like to avoid such extra step.

OK, that's fine.
-- 
Michael

On Mon, Feb 1, 2016 at 9:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Feb 1, 2016 at 9:15 PM, Fujii Masao wrote:
>> If we do that, we also should change the tab-completion for SET command
>> so that "=" is suggested. But I'm afraid that which might decrease that
>> tab-completion.
>>
>> Imagine the case of "SET work_mem <tab>". If "TO" and "=" are suggested,
>> we need to type either "T" or "=" and then <tab> to input the setting value.
>> Otherwise, "SET work_mem <tab>" suggests only "TO" and we can input
>> the setting value by just typing <tab> again.
>> This extra step is very small, but SET command is usually used very often,
>> so I'd like to avoid such extra step.
>
> OK, that's fine.

Pushed. Thanks!

Regards,

-- 
Fujii Masao