public inbox for [email protected]  

help / color / mirror / Atom feed

From: Dagfinn Ilmari Mannsåker <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
Date: Tue, 21 Jan 2025 14:17:03 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAHut+PuD0aQfjAN0q_7FpdNdSEdrhONpTsb0tQi5ZEtZHj6bKg@mail.gmail.com>
	<[email protected]>
	<CAHut+PvKrQXY7kM8ct=HC9xYPC6yhdJWnvCgijtgxmO-mus8uQ@mail.gmail.com>
	<[email protected]>

Michael Paquier <[email protected]> writes:

> On Tue, Jan 21, 2025 at 10:31:01AM +1100, Peter Smith wrote:
>> I applied the v5* patches and ran make check-world. All passed OK.
>> 
>> Your fat comma changes have greatly improved readability, particularly
>> in 040_createsubscriber (the file that caused me to start this
>> thread). Thanks!
>
> It is not an improvement in only 040_createsubscriber, all the places
> Dagfinn is touching here feel much better with this new syntax.

Thanks, I'm glad you agree.

> You have spent a lot of time on the change from short to long names
> and on the new convention grammar for the option/value duos, clearly.

I did it by grepping for the various functions that call programs, such
as command_((fails_)?like|ok) etc, and then reading the docs for each
command to find the corresponding long options.  It got almost
meditative after a while, especially once I got some Emacs keyboard
macros dialled in.

The ones in src/bin/pg_resetwal/t/001_basic.pl I deliberatly left short
because the error messages refer to the short spelling even if you used
the long one.

> It took me some time to read through the patch to catch
> inconsistencies.
>
> -   [ 'pg_verifybackup', '-s', $backup_path ],
> +   [ 'pg_verifybackup', '--skip-checksum', $backup_path ],
>     qr/backup successfully verified/,
>     '-s skips checksumming');
>
> This one in pg_verifybackup should perhaps switch to the long option
> for the test description, like the rest.

Yeah, good point.

> The option should be named --skip-checksums, causing a test failure in
> the CI.

All the tests passed for me on both Debian and macOS, I guess their
getopts accept unambiguous abbreviations of options.

> -my @pg_basebackup_cmd = (@pg_basebackup_defs, '-U', 'backupuser', '-Xfetch');
> +my @pg_basebackup_cmd = (
> +	@pg_basebackup_defs,
> +	'--user' => 'backupuser',
>
> There was a second series of failures in the test
> basebackup_to_shell/t/001_basic.pl because of this part.  The option
> name should be "--username" for pg_basebackup.

Yeah.

> With these two fixes, the CI is happy, which should hopefully be
> enough for the buildfarm.  But we'll see..
>
> I'd like to apply what you have done here (planning to do a second
> lookup, FWIW).  If anybody has objections, feel free to post that
> here.

Thanks again for reviewing this monster patch.

- ilmari

view thread (11+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox