Thread: Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Dagfinn Ilmari Mannsåker
Date:
Peter Smith <smithpb2250@gmail.com> writes:
> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <michael@paquier.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.
- ilmari
From e972f1a5f87499390f401e7db2c079fb87533553 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 12 Dec 2024 11:56:15 +0000
Subject: [PATCH v3] Modify wrapping to make command options easier to read
Use fat comma after options that take values, both to make it clearer
to humans, and to stop perltidy from re-wrapping them.
---
.../t/040_pg_createsubscriber.pl | 169 +++++++++---------
1 file changed, 89 insertions(+), 80 deletions(-)
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0a900edb656..0f7bb103177 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -168,41 +168,44 @@ sub generate_db
# Run pg_createsubscriber on a promoted server
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_t->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_t->host, '--subscriber-port',
- $node_t->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_t->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_t->host,
+ '--subscriber-port' => $node_t->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'target server is not in recovery');
# Run pg_createsubscriber when standby is running
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby is up and running');
# Run pg_createsubscriber on about-to-fail node F
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $node_f->data_dir,
- '--publisher-server', $node_p->connstr($db1),
- '--socketdir', $node_f->host,
- '--subscriber-port', $node_f->port,
- '--database', $db1,
- '--database', $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $node_f->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_f->host,
+ '--subscriber-port' => $node_f->port,
+ '--database' => $db1,
+ '--database' => $db2
],
'subscriber data directory is not a copy of the source database cluster');
@@ -216,14 +219,15 @@ sub generate_db
# Run pg_createsubscriber on node C (P -> S -> C)
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_c->data_dir, '--publisher-server',
- $node_s->connstr($db1), '--socketdir',
- $node_c->host, '--subscriber-port',
- $node_c->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_c->data_dir,
+ '--publisher-server' => $node_s->connstr($db1),
+ '--socketdir' => $node_c->host,
+ '--subscriber-port' => $node_c->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'primary server is in recovery');
@@ -239,14 +243,16 @@ sub generate_db
$node_s->stop;
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
+
],
'primary contains unmet conditions on node P');
# Restore default settings here but only apply it after testing standby. Some
@@ -268,14 +274,15 @@ sub generate_db
});
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby contains unmet conditions on node S');
$node_s->append_conf(
@@ -321,19 +328,20 @@ sub generate_db
# dry run mode on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'pub2', '--subscription',
- 'sub1', '--subscription',
- 'sub2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--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',
+ '--subscription' => 'sub1',
+ '--subscription' => 'sub2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber --dry-run on node S');
@@ -346,32 +354,33 @@ sub generate_db
# pg_createsubscriber can run without --databases option
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--replication-slot',
- 'replslot1'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--replication-slot' => 'replslot1',
],
'run pg_createsubscriber without --databases');
# 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');
--
2.47.1
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Dagfinn Ilmari Mannsåker
Date:
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> Peter Smith <smithpb2250@gmail.com> writes:
>
>> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <michael@paquier.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
From 953d0c8ca8202d6f53af833fd53e3f6b1929fa77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 12 Dec 2024 11:56:15 +0000
Subject: [PATCH v4] Modify wrapping to make command options easier to read
Use fat comma after options that take values, both to make it clearer
to humans, and to stop perltidy from re-wrapping them.
Also remove pointless quoting of $PostgreSQL::Test::Utils::timeout_default.
---
.../t/040_pg_createsubscriber.pl | 255 +++++++++---------
1 file changed, 135 insertions(+), 120 deletions(-)
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0a900edb656..369846db0d0 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -46,69 +46,75 @@ sub generate_db
command_fails(['pg_createsubscriber'],
'no subscriber data directory specified');
command_fails(
- [ 'pg_createsubscriber', '--pgdata', $datadir ],
+ [ 'pg_createsubscriber', '--pgdata' => $datadir ],
'no publisher connection string specified');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
],
'no database name specified');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--database', 'pg1',
- '--database', 'pg1'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--database' => 'pg1',
+ '--database' => 'pg1',
],
'duplicate database name');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--publication', 'foo1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--publication' => 'foo1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'duplicate publication name');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'wrong number of publication names');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--publication', 'foo2',
- '--subscription', 'bar1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--publication' => 'foo2',
+ '--subscription' => 'bar1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'wrong number of subscription names');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--publication', 'foo2',
- '--subscription', 'bar1',
- '--subscription', 'bar2',
- '--replication-slot', 'baz1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--publication' => 'foo2',
+ '--subscription' => 'bar1',
+ '--subscription' => 'bar2',
+ '--replication-slot' => 'baz1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'wrong number of replication slot names');
@@ -168,41 +174,44 @@ sub generate_db
# Run pg_createsubscriber on a promoted server
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_t->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_t->host, '--subscriber-port',
- $node_t->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_t->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_t->host,
+ '--subscriber-port' => $node_t->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'target server is not in recovery');
# Run pg_createsubscriber when standby is running
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby is up and running');
# Run pg_createsubscriber on about-to-fail node F
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $node_f->data_dir,
- '--publisher-server', $node_p->connstr($db1),
- '--socketdir', $node_f->host,
- '--subscriber-port', $node_f->port,
- '--database', $db1,
- '--database', $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $node_f->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_f->host,
+ '--subscriber-port' => $node_f->port,
+ '--database' => $db1,
+ '--database' => $db2
],
'subscriber data directory is not a copy of the source database cluster');
@@ -216,14 +225,15 @@ sub generate_db
# Run pg_createsubscriber on node C (P -> S -> C)
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_c->data_dir, '--publisher-server',
- $node_s->connstr($db1), '--socketdir',
- $node_c->host, '--subscriber-port',
- $node_c->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_c->data_dir,
+ '--publisher-server' => $node_s->connstr($db1),
+ '--socketdir' => $node_c->host,
+ '--subscriber-port' => $node_c->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'primary server is in recovery');
@@ -239,14 +249,16 @@ sub generate_db
$node_s->stop;
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
+
],
'primary contains unmet conditions on node P');
# Restore default settings here but only apply it after testing standby. Some
@@ -268,14 +280,15 @@ sub generate_db
});
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby contains unmet conditions on node S');
$node_s->append_conf(
@@ -321,19 +334,20 @@ sub generate_db
# dry run mode on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'pub2', '--subscription',
- 'sub1', '--subscription',
- 'sub2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--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',
+ '--subscription' => 'sub1',
+ '--subscription' => 'sub2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber --dry-run on node S');
@@ -346,32 +360,33 @@ sub generate_db
# pg_createsubscriber can run without --databases option
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--replication-slot',
- 'replslot1'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--replication-slot' => 'replslot1',
],
'run pg_createsubscriber without --databases');
# 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');
--
2.47.1
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Andrew Dunstan
Date:
On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:Peter Smith <smithpb2250@gmail.com> writes:On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <michael@paquier.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. -- MichaelHi 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.
+1 for this approach.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>> 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.
> +1 for this approach.
Indeed, this is much nicer if it's something perltidy knows about.
However, I know we have the same issue in many other places.
Anyone feel like running through all the TAP scripts?
regards, tom lane
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Dagfinn Ilmari Mannsåker
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>>> 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.
>
>> +1 for this approach.
>
> Indeed, this is much nicer if it's something perltidy knows about.
>
> However, I know we have the same issue in many other places.
> Anyone feel like running through all the TAP scripts?
I can have a go in the next few days. A quick grep spotted another
workaround in 027_stream_regress.pl: using paretheses around the option
and its value:
command_ok(
[
'pg_dump',
('--schema', 'pg_catalog'),
('-f', $outputdir . '/catalogs_primary.dump'),
'--no-sync',
('-p', $node_primary->port),
'--no-unlogged-table-data',
'regression'
],
'dump catalogs of primary server');
I think the fat comma is much nicer than this, so I'd like to convert
these too (and replace some of the concatenations with interpolation).
Technically the quotes aren't necessary around single-dash options
before => since unary minus works on strings as well as numbers, but
I'll leave them in for consistency.
- ilmari
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Andrew Dunstan
Date:
On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>>>> 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.
>>> +1 for this approach.
>> Indeed, this is much nicer if it's something perltidy knows about.
>>
>> However, I know we have the same issue in many other places.
>> Anyone feel like running through all the TAP scripts?
> I can have a go in the next few days. A quick grep spotted another
> workaround in 027_stream_regress.pl: using paretheses around the option
> and its value:
>
> command_ok(
> [
> 'pg_dump',
> ('--schema', 'pg_catalog'),
> ('-f', $outputdir . '/catalogs_primary.dump'),
> '--no-sync',
> ('-p', $node_primary->port),
> '--no-unlogged-table-data',
> 'regression'
> ],
> 'dump catalogs of primary server');
>
> I think the fat comma is much nicer than this, so I'd like to convert
> these too (and replace some of the concatenations with interpolation).
>
> Technically the quotes aren't necessary around single-dash options
> before => since unary minus works on strings as well as numbers, but
> I'll leave them in for consistency.
>
I'd rather get rid of those and just use the long options.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Dagfinn Ilmari Mannsåker
Date:
On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
>>
>> command_ok(
>> [
>> 'pg_dump',
>> ('--schema', 'pg_catalog'),
>> ('-f', $outputdir . '/catalogs_primary.dump'),
>> '--no-sync',
>> ('-p', $node_primary->port),
>> '--no-unlogged-table-data',
>> 'regression'
>> ],
>> 'dump catalogs of primary server');
>>
>> I think the fat comma is much nicer than this, so I'd like to convert
>> these too (and replace some of the concatenations with interpolation).
>>
>> Technically the quotes aren't necessary around single-dash options
>> before => since unary minus works on strings as well as numbers, but
>> I'll leave them in for consistency.
>
> I'd rather get rid of those and just use the long options.
Yeah, that is more self-documenting, so I'll do that while I'm at it.
--
- ilmari
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Peter Smith
Date:
On Fri, Dec 13, 2024 at 5:03 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> > On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> >>
> >> command_ok(
> >> [
> >> 'pg_dump',
> >> ('--schema', 'pg_catalog'),
> >> ('-f', $outputdir . '/catalogs_primary.dump'),
> >> '--no-sync',
> >> ('-p', $node_primary->port),
> >> '--no-unlogged-table-data',
> >> 'regression'
> >> ],
> >> 'dump catalogs of primary server');
> >>
> >> I think the fat comma is much nicer than this, so I'd like to convert
> >> these too (and replace some of the concatenations with interpolation).
> >>
> >> Technically the quotes aren't necessary around single-dash options
> >> before => since unary minus works on strings as well as numbers, but
> >> I'll leave them in for consistency.
> >
> > I'd rather get rid of those and just use the long options.
>
> Yeah, that is more self-documenting, so I'll do that while I'm at it.
>
> --
Your fat-comma solution is much better than something I could have
come up with. Thanks for taking this on.
======
Kind Regards,
Peter Smith.
Fujitsu Australia