Обсуждение: add tab-complete for ALTER DOMAIN ADD...

hi.

per https://www.postgresql.org/docs/current/sql-alterdomain.html

we can add tab-complete for ALTER DOMAIN ADD variants:
ALTER DOMAIN sth ADD CHECK
ALTER DOMAIN sth ADD CONSTRAINT
ALTER DOMAIN sth ADD NOT NULL

jian he <jian.universality@gmail.com> writes:

> hi.
>
> per https://www.postgresql.org/docs/current/sql-alterdomain.html
>
> we can add tab-complete for ALTER DOMAIN ADD variants:
> ALTER DOMAIN sth ADD CHECK
> ALTER DOMAIN sth ADD CONSTRAINT
> ALTER DOMAIN sth ADD NOT NULL

Good catch.

> +    /* ALTER DOMAIN <sth> ADD */
> +    else if (Matches("ALTER", "DOMAIN", MatchAny, "ADD"))
> +        COMPLETE_WITH("CONSTRAINT", "NOT NULL", "CHECK");

I think the completion for CHECK should include the opening paren too,
since that's required for the expression.  We could also add completion
after CONSTRAINT <name>, like this:

    else if(Matches("ALTER", "DOMAIN", MatchAny, "ADD", "CONSTRAINT", MatchAny))
        COMPLETE_WITH("NOT NULL", "CHECK (");

- ilmari

On 2025-Apr-29, Dagfinn Ilmari Mannsåker wrote:

> jian he <jian.universality@gmail.com> writes:

> > +    /* ALTER DOMAIN <sth> ADD */
> > +    else if (Matches("ALTER", "DOMAIN", MatchAny, "ADD"))
> > +        COMPLETE_WITH("CONSTRAINT", "NOT NULL", "CHECK");
> 
> I think the completion for CHECK should include the opening paren too,
> since that's required for the expression.

Yeah, we do that elsewhere.

> We could also add completion after CONSTRAINT <name>, like this:
> 
>     else if(Matches("ALTER", "DOMAIN", MatchAny, "ADD", "CONSTRAINT", MatchAny))
>         COMPLETE_WITH("NOT NULL", "CHECK (");

Done that, and pushed.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos.  Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)

On Sun, 11 May 2025, at 15:48, Álvaro Herrera wrote:
> So we should also have tab-completion for ALTER TABLE ADD NOT NULL, as
> in the attached.  Any opinions?

LGTM

--
- ilmari

On 2025/05/11 23:22, Álvaro Herrera wrote:
> On 2025-Apr-29, Dagfinn Ilmari Mannsåker wrote:
> 
>> jian he <jian.universality@gmail.com> writes:
> 
>>> +    /* ALTER DOMAIN <sth> ADD */
>>> +    else if (Matches("ALTER", "DOMAIN", MatchAny, "ADD"))
>>> +        COMPLETE_WITH("CONSTRAINT", "NOT NULL", "CHECK");
>>
>> I think the completion for CHECK should include the opening paren too,
>> since that's required for the expression.
> 
> Yeah, we do that elsewhere.
> 
>> We could also add completion after CONSTRAINT <name>, like this:
>>
>>     else if(Matches("ALTER", "DOMAIN", MatchAny, "ADD", "CONSTRAINT", MatchAny))
>>         COMPLETE_WITH("NOT NULL", "CHECK (");
> 
> Done that, and pushed.

I have no objection to this commit. However, I had assumed we would
wait to commit changes like this - which aren't bug fixes or
v18-related oversights - until master becomes the development branch
for v19. Maybe I'm missing something..

We can go ahead with this one because it's a small change? Just checking,
since I have a few similar tab-completion improvements patches and
have been holding off until v19 development begins. If it's fine,
I'm thinking of committing them soon as well.

Regards,

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

Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2025/05/11 23:22, Álvaro Herrera wrote:
>> Done that, and pushed.

> I have no objection to this commit. However, I had assumed we would
> wait to commit changes like this - which aren't bug fixes or
> v18-related oversights - until master becomes the development branch
> for v19. Maybe I'm missing something..

I was surprised by this commit too.  Yeah, it's small, but feature
freeze is not about small.

            regards, tom lane

On 2025-May-12, Fujii Masao wrote:

> I have no objection to this commit. However, I had assumed we would
> wait to commit changes like this - which aren't bug fixes or
> v18-related oversights - until master becomes the development branch
> for v19. Maybe I'm missing something..

Yeah, fair question.  We got new syntax to add NOT NULL constraints, and
this adds tab-completion for that.  It does, in addition, add
tab-completion for CHECK constraints and for the CONSTRAINT keyword,
which is much older syntax.  So I guess you could argue that it would
have been okay to add the NOT NULL one (it could be considered an open
item), but not the the other two because they're instead a "new
feature".  But that helps nobody, because we would be offering _some_
options in a command but not all possible ones, which I think is even
worse because it'd be misleading.

Consider the other patch I sent later.  It adds tab-completion coverage
for the new NOT NULL syntax in ALTER TABLE, so it should be fair game
for 18, shouldn't it?  Again: it'd be misleading not to tab-complete
that syntax, because it would confuse people into failing to realize
that that syntax exists, if they see other options but not that one.


As for whether your proposed tab-completion improvements are acceptable
to be included now, I guess it's all subjective on what exactly they
are.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I think my standards have lowered enough that now I think 'good design'
is when the page doesn't irritate the living f*ck out of me." (JWZ)

On 2025-May-11, Álvaro Herrera wrote:

> So we should also have tab-completion for ALTER TABLE ADD NOT NULL, as
> in the attached.  Any opinions?

I have pushed this now, adding the parens after CHECK and reordering the
options per Fujii's suggestion in another thread.  I opted to push only
to master, but there's room to argue that this should be backpatched to
18 since it covers syntax newly added there.  If I get a couple of ayes
and not too many nays, I would do so at some point before beta2.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")