A few comments about v5.

New patch.

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

- 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


- 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


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.

use strict;
use warnings;

use Config;
use PostgresNode;
use TestLib;
use Test::More tests => 15;

# test invalid \if respects ON_ERROR_STOP
my $node = get_new_node('master');

my $tests = [
  [ "\\if invalid_expression\n\\endif\n", '', 'boolean expected', 'syntax error' ],
  # unmatched checks
  [ "\\if true\\n", '', 'found EOF before closing.*endif', 'unmatched \if' ],
  [ "\\elif true\\n", '', 'encountered un-matched.*elif', 'unmatched \elif' ],
  [ "\\else\\n", '', 'encountered un-matched.*else', 'unmatched \else' ],
  [ "\\endif\\n", '', 'encountered un-matched.*endif', 'unmatched \endif' ],

# 3 checks per tests
for my $test (@$tests) {
  my ($script, $stdout_expect, $stderr_re, $name) = @$test;
  my ($stdout, $stderr);
  my $retcode = $node->psql('postgres', $script,
		stdout => \$stdout, stderr => \$stderr,
		on_error_stop => 1);
  is($retcode,'3',"$name test respects ON_ERROR_STOP");
  is($stdout, $stdout_expect, "$name test STDOUT");
  like($stderr, qr/$stderr_re/, "$name test STDERR");


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

Reply via email to