Обсуждение: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

Hi,

I created a patch for COMMENT tab completion.
It was missing TRANSFORM FOR where it's supposed to be.

Best wishes,
Ken Kato

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

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