Re: pg_createsubscriber TAP test wrapping makes command options hard to read.

On Fri, Dec 13, 2024 at 12:17 AM Dagfinn Ilmari Mannsåker
<ilmari(at)ilmari(dot)org> wrote:
>
> Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org> writes:
>
> > Peter Smith <smithpb2250(at)gmail(dot)com> writes:
> >
> >> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >> ...
> >>
> >>> > So, AFAICT I can workaround the perltidy wrapping just by putting all
> >>> > the noarg options at the bottom of the command, then all the
> >>> > option/optarg pairs (ie 2s) will stay together. I can post another
> >>> > patch to do it this way unless you think it is too hacky.
> >>>
> >>> This trick works for me if that makes the long list of option easier
> >>> to read. With two elements of the array perl line, I would just put
> >>> some --dry-run or --verbose at the end of their respective arrays.
> >>> --
> >>> Michael
> >>
> >> Hi Michael.
> >>
> >> Yes, that is the workaround that I was proposing.
> >
> > A better option, IMO, is to use the fat comma (=>) between options and
> > their values. This makes it clear both to humans and perltidy that they
> > belong together, and we can put all the valueless options first without
> > things being rewrapped.
>
> Here's a more thorough patch, that also applies the fat comma treatment
> to other pg_createsubscriber invocations in the same file that don't
> currently happen to be mangled by perltidy. It also adds trailing
> commas to the last item in multi-line command arrays, which is common
> perl style.
>
> - ilmari
>

Hi,

In your v4 patch, there is a fragment (below) that replaces a double
'--verbose' switch with just a single '--verbose'.

As I have only recently learned, the '--verbose'' switch has a
cumulative effect [1], so the original double '--verbose' was probably
deliberate so it should be kept that way.

~~

# Run pg_createsubscriber on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--verbose', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'Pub2', '--replication-slot',
- 'replslot1', '--replication-slot',
- 'replslot2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--publication' => 'pub1',
+ '--publication' => 'pub2',
+ '--replication-slot' => 'replslot1',
+ '--replication-slot' => 'replslot2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber on node S');

======
[1] https://www.postgresql.org/docs/devel/app-pgcreatesubscriber.html

Kind Regards,
Peter Smith.
Fujitsu Australia