Thread: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

[PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
katouknl
Date:
Hi,

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

Best wishes,
Ken Kato
Attachment

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Suraj Khamkar
Date:
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,

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

Best wishes,
Ken Kato

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
katouknl
Date:
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
Attachment

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Shinya Kato
Date:
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



Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Suraj Khamkar
Date:
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 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:
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

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
katouknl
Date:
> 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
Attachment

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Shinya Kato
Date:
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



Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Ken Kato
Date:
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
Attachment

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Shinya Kato
Date:
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
Attachment

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Shinya Kato
Date:
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



Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Fujii Masao
Date:

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



Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Ken Kato
Date:
>>> +   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
Attachment

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Ken Kato
Date:
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
Attachment

Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Daniel Gustafsson
Date:
> 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/




Re: [PATCH] Added TRANSFORM FOR for COMMENT tab completion

From
Fujii Masao
Date:

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