Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, Erik Rijkers <er(at)xs4all(dot)nl>, Daniel Verite <daniel(at)manitou-mail(dot)org>, PostgreSQL <pgsql-hackers(at)postgresql(dot)org>, pgsql-hackers-owner(at)postgresql(dot)org
Subject: Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)
Date: 2017-02-04 07:26:46
Message-ID: alpine.DEB.2.20.1702040739120.20076@lancre
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


A few comments about v5.

> New patch.

Patch applies (with patch, I gave up on "git apply").
Make check ok.
Psql tap test ok.

> Highlights:
> - Interactive barking on branching state changes, commands typed while in
> inactive state

I noticed that the "barking" is conditional to "success". ISTM that it
should always "bark" in interactive mode, whether success or not.

While testing it and seeing the code, I agree that it is too
verbose/redundant. At least remove "active/inactive, ".

> - Help text. New block in help text called "Conditionals"

Maybe it could be moved to "Input/Output" renamed as "Input/Output
Control", or maybe the "Conditionals" section could be moved next to it,
it seems more logical than after large objects.

I think that the descriptions are too long. The interactive user can be
trusted to know what "if/elif/else/endif" mean, or to refer to the full
documentation otherwise. The point is just to provide a syntax and
function reminder, not a substitute for the doc. Thus I would suggest
shorter one-line messages like:

\if <expr> begin a new conditional block
\elif <expr> else if in the current conditional block
\else else in current conditional block
\endif end current conditional block

There should not be a \n at the end, I think, but just between sections.

> - SendQuery calls in mainloop.c are all encapsulated in send_query() to
> ensure the same if-active and if-interactive logic is used

Ok.

> - Exactly one perl TAP test, testing ON_ERROR_STOP. I predict more will be
> needed, but I'm not sure what coverage is desired

More that one:-)

> - I also predict that my TAP test style is pathetic

Hmmm. Perl is perl. Attached an attempt at improving that, which is
probably debatable, but at least it is easy to add further tests without
massive copy-pasting.

> - regression tests now have comments to explain purpose

Ok.

Small details about the code:

+ if (!pset.active_branch && !is_branching_command(cmd) )

Not sure why there is a space before the last closing parenthesis.

--
Fabien.

Attachment Content-Type Size
001_if.pl text/x-perl 1.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Boris Muratshin 2017-02-04 08:41:29 3D Z-curve spatial index
Previous Message Konstantin Knizhnik 2017-02-04 07:19:33 Re: logical decoding of two-phase transactions