Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

On 2021/01/05 15:02, Shinya11(dot)Kato(at)nttdata(dot)com wrote:
> Thank you for your review!
> I fixed some codes and attach a new patch.

Thanks for updating the patch!

+#define Query_for_list_of_cursors \
+" SELECT name FROM pg_cursors"\

This query should be the following?

" SELECT pg_catalog.quote_ident(name) "\
" FROM pg_catalog.pg_cursors "\
" WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'"

+/* CLOSE */
+ else if (Matches("CLOSE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+ " UNION ALL SELECT 'ALL'");

"UNION ALL" should be "UNION"?

+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+ " UNION SELECT 'ABSOLUTE'"
+ " UNION SELECT 'BACKWARD'"
+ " UNION SELECT 'FORWARD'"
+ " UNION SELECT 'RELATIVE'"
+ " UNION SELECT 'ALL'"
+ " UNION SELECT 'NEXT'"
+ " UNION SELECT 'PRIOR'"
+ " UNION SELECT 'FIRST'"
+ " UNION SELECT 'LAST'"
+ " UNION SELECT 'FROM'"
+ " UNION SELECT 'IN'");

This change makes psql unexpectedly output "FROM" and "IN" just after "FETCH".

>
>> When I applied the patch, I got the following whitespace warnings:
>>
>> $ git apply ~/patches/fix_tab_complete_close_fetch_move.patch
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:40:
>> indent with spaces.
>> COMPLETE_WITH_QUERY(Query_for_list_of_cursors
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:41:
>> indent with spaces.
>> " UNION SELECT 'ABSOLUTE'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:42:
>> indent with spaces.
>> " UNION SELECT 'BACKWARD'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:43:
>> indent with spaces.
>> " UNION SELECT 'FORWARD'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:44:
>> indent with spaces.
>> " UNION SELECT 'RELATIVE'"
>> warning: squelched 19 whitespace errors
>> warning: 24 lines add whitespace errors.
>>
>> I recommend you checking whitespaces or running pgindent.
>
> Thank you for your advice and I have corrected it.
>
>> Here are some comments:
>>
>> /*
>> - * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>> RELATIVE, ALL,
>> - * NEXT, PRIOR, FIRST, LAST
>> + * Complete FETCH with a list of cursors and one of ABSOLUTE,
>> BACKWARD, FORWARD, RELATIVE, ALL,
>> + * NEXT, PRIOR, FIRST, LAST, FROM, IN
>> */
>>
>> Maybe I think the commend should say:
>>
>> + * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>> RELATIVE, ALL,
>> + * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors
>>
>> Other updates of the comment seem to have the same issue.
>>
>> Or I think we can omit the details from the comment since it seems redundant
>> information. We can read it from the arguments of the following
>> COMPLETE_WITH_QUERY().
>
> It certainly seems redundant, so I deleted them.

I think that it's better to update and keep those comments rather than removing them.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION