From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Fetter <david@fetter.org>
Cc: Arthur Zakirov <a.zakirov@postgrespro.ru>
Cc: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: Corey Huinker <corey.huinker@gmail.com>
Cc: PostgreSQL Development <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH v4] Add \warn to psql
Date: Tue, 02 Jul 2019 16:10:50 -0400
Message-ID: <11038.1562098250@sss.pgh.pa.us> (raw)
In-Reply-To: <20190501073857.GY28936@fetter.org>
References: <20190427171548.GM28936@fetter.org>
<alpine.DEB.2.21.1904272158120.29865@lancre>
<20190428145813.GO28936@fetter.org>
<alpine.DEB.2.21.1904282015420.29865@lancre>
<20190429041106.GB2954@fetter.org>
<alpine.DEB.2.21.1904290822470.8815@lancre>
<20190429141710.GD2954@fetter.org>
<10811be6-cd8d-eadb-9b70-7a6e72ed4ee5@gmail.com>
<alpine.DEB.2.21.1904301543350.24513@lancre>
<CAKNkYnz8_wOhuHFp953LEvpsStB9Scyikz4HosSk3Q0w1Fa9Pw@mail.gmail.com>
<20190501073857.GY28936@fetter.org>
David Fetter <david@fetter.org> writes:
> [ v7-0001-Add-warn-to-psql.patch ]
I took a look at this. I have no quibble with the proposed feature,
and the implementation is certainly simple enough. But I'm unconvinced
about the proposed test scaffolding. Spinning up a new PG instance is a
*hell* of a lot of overhead to pay for testing something that could be
tested as per attached. Admittedly, the attached doesn't positively
prove which pipe each output string went down, but that does not strike
me as a concern large enough to justify adding a TAP test for.
I'd be happier about adding TAP infrastructure if it looked like it'd
be usable to test some of the psql areas that are unreachable by the
existing test methodology, particularly tab-complete.c and prompt.c.
But I don't see anything here that looks like it'll work for that.
I don't like what you did to command_checks_all, either --- it could
hardly say "bolted on after the fact" more clearly if you'd written
that in <blink><red> text. If we need an input-stream argument,
let's just add it in a rational place and adjust the callers.
There aren't that many of 'em, nor has the subroutine been around
all that long.
regards, tom lane
Attachments:easier-warn-test.patch (text/x-diff, 1.5K) downloaddiff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.outindex 4bcf0cc..9b4060f 100644--- a/src/test/regress/expected/psql.out+++ b/src/test/regress/expected/psql.out@@ -4180,6 +4180,29 @@ drop table psql_serial_tab;
\pset format aligned
\pset expanded off
\pset border 1
+-- \echo and allied features+\echo this is a test+this is a test+\echo -n this is a test without newline+this is a test without newline\echo more test+more test+\set foo bar+\echo foo = :foo+foo = bar+\qecho this is a test+this is a test+\qecho -n this is a test without newline+this is a test without newline\qecho more test+more test+\qecho foo = :foo+foo = bar+\warn this is a test+this is a test+\warn -n this is a test without newline+this is a test without newline\warn more test+more test+\warn foo = :foo+foo = bar
-- tests for \if ... \endif
\if true
select 'okay';
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sqlindex 26f436a..2bae6c5 100644--- a/src/test/regress/sql/psql.sql+++ b/src/test/regress/sql/psql.sql@@ -771,6 +771,24 @@ drop table psql_serial_tab;
\pset expanded off
\pset border 1
+-- \echo and allied features++\echo this is a test+\echo -n this is a test without newline+\echo more test+\set foo bar+\echo foo = :foo++\qecho this is a test+\qecho -n this is a test without newline+\qecho more test+\qecho foo = :foo++\warn this is a test+\warn -n this is a test without newline+\warn more test+\warn foo = :foo+
-- tests for \if ... \endif
\if true
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, david@fetter.org, a.zakirov@postgrespro.ru, coelho@cri.ensmp.fr, corey.huinker@gmail.com
Subject: Re: [PATCH v4] Add \warn to psql
In-Reply-To: <11038.1562098250@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox