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

On Tue, Jan 5, 2021 at 3:03 PM <Shinya11(dot)Kato(at)nttdata(dot)com> wrote:
>
> Thank you for your review!
> I fixed some codes and attach a new patch.

Thank you for updating the patch!

>
> >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.
>
> >---
> >- /*
> >- * Complete FETCH <direction> with "FROM" or "IN". These are equivalent,
> >- * but we may as well tab-complete both: perhaps some users prefer one
> >- * variant or the other.
> >- */
> >+ /* Complete FETCH <direction> with a list of cursors and "FROM" or
> >+ "IN" */
> >
> >Why did you remove the second sentence in the above comment?
>
> I had misunderstood the meaning and deleted it.
> I deleted it as well as above, but would you prefer it to be there?

I would leave it. I realized this area is recently updated by commit
8176afd8b7. In that change, the comments were updated rather than
removed. So it might be better to leave them. Sorry for confusing you.

>
> >---
> >The patch improves tab completion for CLOSE, FETCH, and MOVE but is there
> >any reason why you didn't do that for DECLARE? I think DECLARE also can be
> >improved and it's a good timing for that.
>
> I wanted to improve tab completion for DECLARE, but I couldn't find anything to improve.
> Please let me know if there are any codes that can be improved.

I've attached the patch improving the tab completion for DECLARE as an
example. What do you think?

BTW according to the documentation, the options of DECLARE statement
(BINARY, INSENSITIVE, SCROLL, and NO SCROLL) are order-sensitive.

DECLARE name [ BINARY ] [ INSENSITIVE ] [ [ NO ] SCROLL ]
CURSOR [ { WITH | WITHOUT } HOLD ] FOR query

But I realized that these options are actually order-insensitive. For
instance, we can declare a cursor like:

=# declare abc scroll binary cursor for select * from pg_class;
DECLARE CURSOR

The both parser code and documentation has been unchanged from 2003.
Is it a documentation bug?

Regards,

--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/