[PATCH] Added TRANSFORM FOR for COMMENT tab completion

Hello,
I reviewed your patch. At a first glance, I have the below comments.

   1. The below change crosses the 80-character limit in a line. Please
   maintain the same.
   -   "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "ROLE");
   +   "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "TRANSFORM FOR",
   "ROLE");
   2. There are trailing whitespaces after
   COMPLETE_WITH_QUERY(Query_for_list_of_languages);.
   Remove these extra whitespaces.
   surajkhamkar@localhost:tab_comment$ git apply
   fix_tab_complete_comment.patch
   fix_tab_complete_comment.patch:38: trailing whitespace.
   COMPLETE_WITH_QUERY(Query_for_list_of_languages);
   warning: 1 line adds whitespace errors.

Regards,
Suraj Khamkar

On Wed, Sep 29, 2021 at 2:04 PM katouknl <katouknl@oss.nttdata.com> wrote:

Hi,

Thank you for the review.

I wasn't quite sure where to start counting the characters,
but I used pgindent to properly format it, so hopefully everything is
okay.
Also, I sorted them in alphabetical order just to make it look prettier.

* The below change crosses the 80-character limit in a line. Please
maintain the same.
-   "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "ROLE");
+   "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "TRANSFORM FOR",
"ROLE");

I made sure there is no whitespaces this time.

* There are trailing whitespaces after
COMPLETE_WITH_QUERY(Query_for_list_of_languages);.
Remove these extra whitespaces.
surajkhamkar@localhost:tab_comment$ git apply
fix_tab_complete_comment.patch
fix_tab_complete_comment.patch:38: trailing whitespace.
COMPLETE_WITH_QUERY(Query_for_list_of_languages);
warning: 1 line adds whitespace errors.

Best wishes,
Ken Kato

On 2021-10-07 17:14, katouknl wrote:

Hi,

Thank you for the review.

I wasn't quite sure where to start counting the characters,
but I used pgindent to properly format it, so hopefully everything is
okay.
Also, I sorted them in alphabetical order just to make it look
prettier.

* The below change crosses the 80-character limit in a line. Please
maintain the same.
-   "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "ROLE");
+   "LARGE OBJECT", "TABLESPACE", "TEXT SEARCH", "TRANSFORM FOR",
"ROLE");

I made sure there is no whitespaces this time.

* There are trailing whitespaces after
COMPLETE_WITH_QUERY(Query_for_list_of_languages);.
Remove these extra whitespaces.
surajkhamkar@localhost:tab_comment$ git apply
fix_tab_complete_comment.patch
fix_tab_complete_comment.patch:38: trailing whitespace.
COMPLETE_WITH_QUERY(Query_for_list_of_languages);
warning: 1 line adds whitespace errors.

Thank you for the patch!
It is very good, but it seems to me that there are some tab-completion
missing in COMMENT command.
For example,
- CONSTRAINT ... ON DOMAIN
- OPERATOR CLASS
- OPERATOR FAMILY
- POLICY ... ON
- [PROCEDURAL]
- RULE ... ON
- TRIGGER ... ON

I think these tab-comletion also can be improved and it's a good timing
for that.

--
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hello,
Thanks for the revised patch.

It is very good, but it seems to me that there are some tab-completion

missing in COMMENT command.

Thanks Shinya, for having a look. I was also about to say that it would be
good
if we take care of tab-completion for other options as well in this patch
itself.
I would like to ask @katouknl <katouknl@oss.nttdata.com> if it's ok to do
so.

And regarding the revised patch, arranging options in alphabetical order
seems
good to me. Though, there is one line where it crosses 80 characters in a
line.
+ COMPLETE_WITH("ACCESS METHOD", "AGGREGATE", "CAST", "COLLATION", "COLUMN",

Apart from this I don't have any major comment.

Regards,
Suraj Khamkar

On Thu, Oct 7, 2021 at 3:29 PM Shinya Kato <Shinya11.Kato@oss.nttdata.com>
wrote:

It is very good, but it seems to me that there are some tab-completion
missing in COMMENT command.
For example,
- CONSTRAINT ... ON DOMAIN
- OPERATOR CLASS
- OPERATOR FAMILY
- POLICY ... ON
- [PROCEDURAL]
- RULE ... ON
- TRIGGER ... ON

I think these tab-comletion also can be improved and it's a good
timing for that.

Thank you for the comments!

I fixed where you pointed out.

Best wishes,
Ken Kato

On 2021-10-14 14:30, katouknl wrote:

It is very good, but it seems to me that there are some tab-completion
missing in COMMENT command.
For example,
- CONSTRAINT ... ON DOMAIN
- OPERATOR CLASS
- OPERATOR FAMILY
- POLICY ... ON
- [PROCEDURAL]
- RULE ... ON
- TRIGGER ... ON

I think these tab-comletion also can be improved and it's a good
timing for that.

Thank you for the comments!

I fixed where you pointed out.

Thank you for the update!
I tried "COMMENT ON OPERATOR ...", and an operator seemed to be
complemented with double quotation marks.
However, it caused the COMMENT command to fail.
---
postgres=# COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
ERROR: syntax error at or near "("
LINE 1: COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
postgres=# COMMENT ON OPERATOR + (integer, integer) IS 'test_success';
COMMENT
---

So, I think as with \do command, you do not need to complete the
operators.
Do you think?

--
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021-10-15 13:29 に Shinya Kato さんは書きました:

On 2021-10-14 14:30, katouknl wrote:

It is very good, but it seems to me that there are some
tab-completion
missing in COMMENT command.
For example,
- CONSTRAINT ... ON DOMAIN
- OPERATOR CLASS
- OPERATOR FAMILY
- POLICY ... ON
- [PROCEDURAL]
- RULE ... ON
- TRIGGER ... ON

I think these tab-comletion also can be improved and it's a good
timing for that.

Thank you for the comments!

I fixed where you pointed out.

Thank you for the update!
I tried "COMMENT ON OPERATOR ...", and an operator seemed to be
complemented with double quotation marks.
However, it caused the COMMENT command to fail.
---
postgres=# COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
ERROR: syntax error at or near "("
LINE 1: COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
postgres=# COMMENT ON OPERATOR + (integer, integer) IS 'test_success';
COMMENT
---

So, I think as with \do command, you do not need to complete the
operators.
Do you think?

Thank you for the further comments!

I fixed so that it doesn't complete the operators anymore.
It only completes with CLASS and FAMILY.

Also, I updated TEXT SEARCH.
It completes object names for each one of CONFIGURATION, DICTIONARY,
PARSER, and TEMPLATE.

--
Best wishes,

Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021-10-15 17:49, Ken Kato wrote:

2021-10-15 13:29 に Shinya Kato さんは書きました:

On 2021-10-14 14:30, katouknl wrote:

It is very good, but it seems to me that there are some
tab-completion
missing in COMMENT command.
For example,
- CONSTRAINT ... ON DOMAIN
- OPERATOR CLASS
- OPERATOR FAMILY
- POLICY ... ON
- [PROCEDURAL]
- RULE ... ON
- TRIGGER ... ON

I think these tab-comletion also can be improved and it's a good
timing for that.

Thank you for the comments!

I fixed where you pointed out.

Thank you for the update!
I tried "COMMENT ON OPERATOR ...", and an operator seemed to be
complemented with double quotation marks.
However, it caused the COMMENT command to fail.
---
postgres=# COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
ERROR: syntax error at or near "("
LINE 1: COMMENT ON OPERATOR "+" (integer, integer) IS 'test_fail';
postgres=# COMMENT ON OPERATOR + (integer, integer) IS 'test_success';
COMMENT
---

So, I think as with \do command, you do not need to complete the
operators.
Do you think?

Thank you for the further comments!

I fixed so that it doesn't complete the operators anymore.
It only completes with CLASS and FAMILY.

Also, I updated TEXT SEARCH.
It completes object names for each one of CONFIGURATION, DICTIONARY,
PARSER, and TEMPLATE.

Thank you for update!
The patch looks good to me. I applied cosmetic changes to it.
Attached is the updated version of the patch.

Barring any objection, I will change status to Ready for Committer.

--
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Oct 26, 2021 at 05:04:24PM +0900, Shinya Kato wrote:

Barring any objection, I will change status to Ready for Committer.

+   else if (Matches("COMMENT", "ON", "PROCEDURAL"))
+       COMPLETE_WITH("LANGUAGE");
+   else if (Matches("COMMENT", "ON", "PROCEDURAL", "LANGUAGE"))
+       COMPLETE_WITH_QUERY(Query_for_list_of_languages);
I don't think that there is much point in being this picky either with
the usage of PROCEDURAL, as we already complete a similar and simpler
grammar with LANGUAGE.  I would just remove this part of the patch.

+   else if (Matches("COMMENT", "ON", "OPERATOR"))
+       COMPLETE_WITH("CLASS", "FAMILY");
Isn't this one wrong?  Operators can have comments, and we'd miss
them.  This is mentioned upthread, but it seems to me that we'd better
drop this part of the patch if the operator naming part cannot be
solved easily.
--
Michael

On 2021-10-27 14:45, Michael Paquier wrote:

On Tue, Oct 26, 2021 at 05:04:24PM +0900, Shinya Kato wrote:

Barring any objection, I will change status to Ready for Committer.

+   else if (Matches("COMMENT", "ON", "PROCEDURAL"))
+       COMPLETE_WITH("LANGUAGE");
+   else if (Matches("COMMENT", "ON", "PROCEDURAL", "LANGUAGE"))
+       COMPLETE_WITH_QUERY(Query_for_list_of_languages);
I don't think that there is much point in being this picky either with
the usage of PROCEDURAL, as we already complete a similar and simpler
grammar with LANGUAGE.  I would just remove this part of the patch.

In my opinion, it is written in the documentation, so tab-completion of
"PROCEDURAL"is good.
How about a completion with "LANGUAGE" and "PROCEDURAL LANGUAGE", like
"PASSWORD" and "ENCRYPTED PASSWORD" in CREATE ROLE?

+   else if (Matches("COMMENT", "ON", "OPERATOR"))
+       COMPLETE_WITH("CLASS", "FAMILY");
Isn't this one wrong?  Operators can have comments, and we'd miss
them.  This is mentioned upthread, but it seems to me that we'd better
drop this part of the patch if the operator naming part cannot be
solved easily.

As you said, it may be misleading.
I agree to drop it.

--
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Oct 27, 2021 at 03:54:03PM +0900, Shinya Kato wrote:

In my opinion, it is written in the documentation, so tab-completion of
"PROCEDURAL"is good.

It does not mean that we need to add everything either, especially
here as there is a shorter option.

How about a completion with "LANGUAGE" and "PROCEDURAL LANGUAGE", like
"PASSWORD" and "ENCRYPTED PASSWORD" in CREATE ROLE?

This has been around for some time already, so I'd just leave those
parts be.
--
Michael

On 2021/10/27 15:54, Shinya Kato wrote:

On 2021-10-27 14:45, Michael Paquier wrote:

On Tue, Oct 26, 2021 at 05:04:24PM +0900, Shinya Kato wrote:

Barring any objection, I will change status to Ready for Committer.

+   else if (Matches("COMMENT", "ON", "PROCEDURAL"))
+       COMPLETE_WITH("LANGUAGE");
+   else if (Matches("COMMENT", "ON", "PROCEDURAL", "LANGUAGE"))
+       COMPLETE_WITH_QUERY(Query_for_list_of_languages);
I don't think that there is much point in being this picky either with
the usage of PROCEDURAL, as we already complete a similar and simpler
grammar with LANGUAGE.  I would just remove this part of the patch.

In my opinion, it is written in the documentation, so tab-completion of "PROCEDURAL"is good.
How about a completion with "LANGUAGE" and "PROCEDURAL LANGUAGE", like "PASSWORD" and "ENCRYPTED PASSWORD" in CREATE ROLE?

+   else if (Matches("COMMENT", "ON", "OPERATOR"))
+       COMPLETE_WITH("CLASS", "FAMILY");
Isn't this one wrong?  Operators can have comments, and we'd miss
them.  This is mentioned upthread, but it seems to me that we'd better
drop this part of the patch if the operator naming part cannot be
solved easily.

As you said, it may be misleading.
I agree to drop it.

So I changed the status of the patch to Waiting on Author in CF.

+static const SchemaQuery Query_for_list_of_text_search_configurations = {

We already have Query_for_list_of_ts_configurations in tab-complete.c.
Do we really need both queries? Or we can drop either of them?

+#define Query_for_list_of_operator_class_index_methods \
+"SELECT pg_catalog.quote_ident(amname)"\
+"  FROM pg_catalog.pg_am"\
+" WHERE (%d = pg_catalog.length('%s'))"\
+"   AND oid IN "\
+"       (SELECT opcmethod FROM pg_catalog.pg_opclass "\
+"         WHERE pg_catalog.quote_ident(opcname)='%s')"

Isn't it overkill to tab-complete this? I thought that because
I'm not sure if COMMENT command for OPERATOR CLASS or FAMILY is
usually executed via psql interactively, instead I just guess
it's executed via script. Also because there is no tab-completion
support for ALTER OPERATOR CLASS or FAMILY command. It's a bit
strange to support the tab-complete for COMMENT for OPERATOR CLASS
or FAMILY first.

Regards,

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

+   else if (Matches("COMMENT", "ON", "PROCEDURAL"))
+       COMPLETE_WITH("LANGUAGE");
+   else if (Matches("COMMENT", "ON", "PROCEDURAL", "LANGUAGE"))
+       COMPLETE_WITH_QUERY(Query_for_list_of_languages);
I don't think that there is much point in being this picky either 
with
the usage of PROCEDURAL, as we already complete a similar and simpler
grammar with LANGUAGE.  I would just remove this part of the patch.

In my opinion, it is written in the documentation, so tab-completion
of "PROCEDURAL"is good.
How about a completion with "LANGUAGE" and "PROCEDURAL LANGUAGE", like
"PASSWORD" and "ENCRYPTED PASSWORD" in CREATE ROLE?

I kept LANGUAGE and PROCEDURAL LANGUAGE just like PASSWORD and ENCRYPTED
PASSWORD.

+   else if (Matches("COMMENT", "ON", "OPERATOR"))
+       COMPLETE_WITH("CLASS", "FAMILY");
Isn't this one wrong?  Operators can have comments, and we'd miss
them.  This is mentioned upthread, but it seems to me that we'd 
better
drop this part of the patch if the operator naming part cannot be
solved easily.

As you said, it may be misleading.
I agree to drop it.

Hearing all the opinions given, I decided not to support OPERATOR CLASS
or FAMILY in COMMENT.
Therefore, I drooped Query_for_list_of_operator_class_index_methods as
well.

+static const SchemaQuery Query_for_list_of_text_search_configurations
= {

We already have Query_for_list_of_ts_configurations in tab-complete.c.
Do we really need both queries? Or we can drop either of them?

Thank you for pointing out!
I didn't notice that there already exists
Query_for_list_of_ts_configurations,
so I changed TEXT SEARCH completion with using Query_for_list_of_ts_XXX.

I made the changes to the points above and updated the patch.

--
Best wishes,

Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hi,

I found unnecessary line deletion in my previous patch, so I made a
minor update for that.

--
Best wishes,

Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, Nov 05, 2021 at 12:31:42PM +0900, Ken Kato wrote:

I found unnecessary line deletion in my previous patch, so I made a minor
update for that.

I have looked at this version, and this is much simpler than what was
proposed upthread. This looks good, so applied after fixing a couple
of indentation issues in the list of objects after COMMENT ON.
--
Michael

On 5 Nov 2021, at 07:30, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Nov 05, 2021 at 12:31:42PM +0900, Ken Kato wrote:

I found unnecessary line deletion in my previous patch, so I made a minor
update for that.

I have looked at this version, and this is much simpler than what was
proposed upthread. This looks good, so applied after fixing a couple
of indentation issues in the list of objects after COMMENT ON.

Is there anything left on this or can we close it in the commitfest?

--
Daniel Gustafsson https://vmware.com/

On Fri, Nov 05, 2021 at 10:39:36AM +0100, Daniel Gustafsson wrote:

Is there anything left on this or can we close it in the commitfest?

Oops. I have missed that there was a CF entry for this patch, and
missed that Fujii-san was registered as a committer for it. My
apologies!
--
Michael

On 2021/11/05 21:35, Michael Paquier wrote:

On Fri, Nov 05, 2021 at 10:39:36AM +0100, Daniel Gustafsson wrote:

Is there anything left on this or can we close it in the commitfest?

Oops. I have missed that there was a CF entry for this patch, and
missed that Fujii-san was registered as a committer for it. My
apologies!

No problem. Thanks for committing the patch!

Regards,

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