Re: [PATCH] psql: Add tab-complete for optional view parameters

Hi Christoph,

Thanks for the patch! I just tested it and I could reproduce the 
expected behaviour in these cases:

postgres=# CREATE VIEW w
AS      WITH (

postgres=# CREATE OR REPLACE VIEW w
AS      WITH (

postgres=# CREATE VIEW w WITH (
CHECK_OPTION      SECURITY_BARRIER  SECURITY_INVOKER

postgres=# CREATE OR REPLACE VIEW w WITH (
CHECK_OPTION      SECURITY_BARRIER  SECURITY_INVOKER

postgres=# ALTER VIEW w
ALTER COLUMN  OWNER TO      RENAME        RESET (       SET (         
SET SCHEMA

postgres=# ALTER VIEW w RESET (
CHECK_OPTION      SECURITY_BARRIER  SECURITY_INVOKER

postgres=# ALTER VIEW w SET (check_option =
CASCADED  LOCAL

However, an "ALTER TABLE  S" does not complete the open 
parenthesis "(" from "SET (", as suggested in "ALTER VIEW  ".

postgres=# ALTER VIEW w SET
Display all 187 possibilities? (y or n)

Is it intended to behave like this? If so, an "ALTER VIEW  
RES" does complete the open parenthesis -> "RESET (".

Best,
Jim

On 09.12.22 11:31, Christoph Heiss wrote:
> Thanks for the review!
>
> On 12/8/22 12:19, Melih Mutlu wrote:
>> Hi Christoph,
>>
>> I just took a quick look at your patch.
>> Some suggestions:
>>
>>     +   else if (Matches("ALTER", "VIEW", MatchAny, "SET", "("))
>>     +       COMPLETE_WITH_LIST(view_optional_parameters);
>>     +   /* ALTER VIEW xxx RESET ( yyy , ... ) */
>>     +   else if (Matches("ALTER", "VIEW", MatchAny, "RESET", "("))
>>     +       COMPLETE_WITH_LIST(view_optional_parameters);
>>
>>
>> What about combining these two cases into one like Matches("ALTER", 
>> "VIEW", MatchAny, "SET|RESET", "(") ?
> Good thinking, incorporated that into v2.
> While at it, I also added completion for the values of the SET 
> parameters, since that is useful as well.
>
>>
>>          /* ALTER VIEW  */
>>          else if (Matches("ALTER", "VIEW", MatchAny))
>>              COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
>>                            "SET SCHEMA");
>>
>>
>> Also seems like SET and RESET don't get auto-completed for "ALTER 
>> VIEW ".
>> I think it would be nice to include those missing words.
> That is already contained in the patch:
>
> @@ -2139,7 +2146,7 @@ psql_completion(const char *text, int start, int 
> end)
>      /* ALTER VIEW  */
>      else if (Matches("ALTER", "VIEW", MatchAny))
>          COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
> -                      "SET SCHEMA");
> +                      "SET SCHEMA", "SET (", "RESET (");
>
> Thanks,
> Christoph