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

[Corey Huinker]

>
>
> As I understood it, the negative feeback was really about sh inspiration
> "if/fi", not about elif/elsif/elseif. I do not think that there was an
> expressed consensus about the later.
>

True


> The closest case is CPP with its line-oriented #-prefix syntax, and I
> still think that we should do it like that.


Given that the pl/pgsql syntax has a space between "end" and "if", I have
to agree. It's easy enough to change back if others can make a convincing
argument for something else.

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

[Fabien COELHO]

1: unrecognized value "whatever" for "\if"; assuming "on"

I do not think that the script should continue with such an assumption.


I agree, and this means we can't use ParseVariableBool() as-is. I 
already broke out argument reading to it's own function, knowing that 
it'd be the stub for expressions. So I guess we start that now. What 
subset of true-ish values do you think we should support? If we think 
that real expressions are possible soon, we could only allow 'true' and 
'false' for now, but if we expect that expressions might not make it 
into v10, then perhaps we should support the same text values that 
coerce to booleans on the server side.


Hmmm. I would text value that coerce to true? I would also accept non-zero 
integers (eg SELECT 1::BOOL; -- t).


I would suggest to assume false on everything else, and/or maybe to ignore 
the whole if/endif section in such cases.



All valid issues. Will add those to the regression as well (with
ON_ERROR_STOP disabled, obviously).


ISTM that with TAP test you can check for error returns, so maybe this can 
be done.


--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Fabien COELHO]

Hello,


I'm personally indifferent to elif vs elsif vs elseif, but I think elseif
was the consensus. It's easy enough to change.


Went with elsif to follow pl/pgsql. In reviewing the original thread it
seemed that "elif" was linked to "fi" and that got some negative feedback.


As I understood it, the negative feeback was really about sh inspiration 
"if/fi", not about elif/elsif/elseif. I do not think that there was an

expressed consensus about the later.

Else-if variants from different languages include:

 VB: If ElseIf Else End If (with optional Then)
 PHP: if {} elseif {} else {}
 Tcl: if {} elseif {} else {}

 PL/pgSQL: IF ... THEN ... ELSIF ... ELSE ... END IF;
 Perl: if {} elsif {} else {}
 Ruby: if elsif else end

 CPP: #if #elif #else #endif
 Python: if : elif: else:
 bash: if [] then ... elif ... else ... fi

The closest case is CPP with its line-oriented #-prefix syntax, and I 
still think that we should do it like that.



I went looking for other examples of explicit /* PASSTHROUGH */ comments
and could find none. Could you explain what it is you want me to fix with
the switch statements?


Sorry, I got it wrong. The comment (which is FALLTHROUGH or FALL THROUGH, 
not PASSTHROUGH) is required if there is specific code in a case and the 
case is expected to continue with executing the next case code as well. 
This is quite rare, and it is not the case for your code, which just has 
some comments in one case.


--
Fabien


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Corey Huinker]

On Mon, Jan 23, 2017 at 4:12 PM, Corey Huinker 
wrote:

> I do not like the choice of "elseif", which exists in PHP & VB. PL/pgSQL
>> has "ELSIF" like perl, that I do not like much, though. Given the syntax
>> and behavioral proximity with cpp, I suggest that "elif" would be a better
>> choice.
>>
>
> I'm personally indifferent to elif vs elsif vs elseif, but I think elseif
> was the consensus. It's easy enough to change.
>

Went with elsif to follow pl/pgsql. In reviewing the original thread it
seemed that "elif" was linked to "fi" and that got some negative feedback.


>
>
>>
>> Documentation: I do not think that the systematic test-like example in
>> the documentation is relevant, a better example should be shown. The list
>> of what is considered true or false should be told explicitely, not end
>> with "etc" that is everyone to guess.
>>
>
> It was copied from the regression test. I knew there would be follow-up
> suggestions for what should be shown.
>

Work on this is pending discussion of what true/false values we should
allow, and if values outside of those is an error.


>>
>>   case ...: case ...:
>>
>> The project rules are to add explicit /* PASSTHROUGH */ comment.
>>
>
> Will do.
>

I went looking for other examples of explicit /* PASSTHROUGH */ comments
and could find none. Could you explain what it is you want me to fix with
the switch statements?

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

[Corey Huinker]

>
> I do not like the choice of "elseif", which exists in PHP & VB. PL/pgSQL
> has "ELSIF" like perl, that I do not like much, though. Given the syntax
> and behavioral proximity with cpp, I suggest that "elif" would be a better
> choice.
>

I'm personally indifferent to elif vs elsif vs elseif, but I think elseif
was the consensus. It's easy enough to change.


>
> Documentation: I do not think that the systematic test-like example in the
> documentation is relevant, a better example should be shown. The list of
> what is considered true or false should be told explicitely, not end with
> "etc" that is everyone to guess.
>

It was copied from the regression test. I knew there would be follow-up
suggestions for what should be shown.


>
> Tests: On principle they should include echos in all non executed branches
> to reassure that they are indeed not executed.
>

Will do.


>
> Also, no error cases are tested. They should be. Maybe it is not very easy
> to test some cases which expect to make psql generate errors, but I feel
> they are absolutely necessary for such a feature.
>

Will do.


>
> 1: unrecognized value "whatever" for "\if"; assuming "on"
>
> I do not think that the script should continue with such an assumption.
>

I agree, and this means we can't use ParseVariableBool() as-is. I already
broke out argument reading to it's own function, knowing that it'd be the
stub for expressions. So I guess we start that now. What subset of true-ish
values do you think we should support? If we think that real expressions
are possible soon, we could only allow 'true' and 'false' for now, but if
we expect that expressions might not make it into v10, then perhaps we
should support the same text values that coerce to booleans on the server
side.


>
> 2: encountered \else after \else ... "# ERROR BEFORE"
>
> Even with ON_ERROR_STOP activated the execution continues.


> 3: no \endif
>
> Error reported, but it does not stop even with ON_ERROR_STOP.
>
> 4: include with bad nesting...
>
> Again, even with ON_ERROR_STOP, the execution continues...
>

All valid issues. Will add those to the regression as well (with
ON_ERROR_STOP disabled, obviously).


>
>
> Code:
>
>   -   if (status != PSQL_CMD_ERROR)
>   +   if ((status != PSQL_CMD_ERROR) && psqlscan_branch_active(scan_st
> ate))
>
> Why the added parenthesis?
>

Will fix.


>
>   case ...: case ...:
>
> The project rules are to add explicit /* PASSTHROUGH */ comment.
>

Will do.


>
> There are spaces at the end of one line in a comment about
> psqlscan_branch_end_state.
>
> There are multiple "TODO" comments in the file... Is it done or work in
> progress?
>

I forgot to remove them. But it would be wildly optimistic of me to think
there would be no more work for me after the first patch submission.


>
> Usually in pg comments about a function do not repeat the function name.
> For very short comment, they are /* inlined */ on one line. Use pg comment
> style.


In that case, I was copying the style found in other functions psqlscan.l -
I'm happy to remove it.

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

[Fabien COELHO]

A few comments:


Argh, better with the attachements:-(

--
Fabien.

if-error-1.sql
Description: application/sql


if-error-2.sql
Description: application/sql


if-error-3.sql
Description: application/sql


if-error-4.sql
Description: application/sql

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Fabien COELHO]

And here's the patch. I've changed the subject line and will be submitting
a new entry to the commitfest.


A few comments:

Patch applies, make check is ok, but currently really too partial.

I do not like the choice of "elseif", which exists in PHP & VB. PL/pgSQL 
has "ELSIF" like perl, that I do not like much, though. Given the syntax 
and behavioral proximity with cpp, I suggest that "elif" would be a better 
choice.


Documentation: I do not think that the systematic test-like example in the 
documentation is relevant, a better example should be shown. The list of 
what is considered true or false should be told explicitely, not end with 
"etc" that is everyone to guess.


Tests: On principle they should include echos in all non executed branches 
to reassure that they are indeed not executed.


Also, no error cases are tested. They should be. Maybe it is not very easy 
to test some cases which expect to make psql generate errors, but I feel 
they are absolutely necessary for such a feature.


1: unrecognized value "whatever" for "\if"; assuming "on"

I do not think that the script should continue with such an assumption.

2: encountered \else after \else ... "# ERROR BEFORE"

Even with ON_ERROR_STOP activated the execution continues.

3: no \endif

Error reported, but it does not stop even with ON_ERROR_STOP.

4: include with bad nesting...

Again, even with ON_ERROR_STOP, the execution continues...


Code:

  -   if (status != PSQL_CMD_ERROR)
  +   if ((status != PSQL_CMD_ERROR) && psqlscan_branch_active(scan_state))

Why the added parenthesis?

  case ...: case ...:

The project rules are to add explicit /* PASSTHROUGH */ comment.

There are spaces at the end of one line in a comment about 
psqlscan_branch_end_state.


There are multiple "TODO" comments in the file... Is it done or work in 
progress?


Usually in pg comments about a function do not repeat the function name. 
For very short comment, they are /* inlined */ on one line. Use pg comment 
style.


--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers