On 2023-08-21 13:08, Masahiro Ikeda wrote:
Thanks for your review!
(1)
Why don't you add test for the purpose? It could be overkill...
I though the following function is the best place.
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm
b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f..1bdb81ac56 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -843,6 +843,10 @@ sub program_help_ok
ok($result, "$cmd --help exit code 0");
isnt($stdout, '', "$cmd --help goes to stdout");
is($stderr, '', "$cmd --help nothing to stderr");
+ foreach my $line (split /\n/, $stdout)
+ {
+ ok(length($line) <= 80, "$cmd --help output fit within
80 columns per line");
+ }
return;
}
Agreed.
(2)
Is there any reason that only src/bin commands are targeted? I found
that
we also need to fix vacuumlo with the above test. I think it's better
to
fix it because it's a contrib module.
$ vacuumlo --help | while IFS='' read line; do echo $((`echo $line |
wc -m` - 1)) $line; done | sort -n -r | head -n 2
84 -n, --dry-run don't remove large objects, just show
what would be done
74 -l, --limit=LIMIT commit after removing each LIMIT large
objects
This is because I wasn't sure making all --help outputs fit into 80
columns per line is right thing to do as described below:
| If this is the way to go, I'll do same things for contrib commands.
If there are no objection, I'm going to make other commands fit within
80 columns per line including (4).
(3)
Is to delete '/mnt/server' intended? I though it better to leave it as
is since archive_cleanup_command example uses the absolute path.
- " pg_archivecleanup /mnt/server/archiverdir
000000010000000000000010.00000020.backup\n"));
+ " pg_archivecleanup archiverdir
000000010000000000000010.00000020.backup\n"));
I will confirmed that the --help text are not changed and only
the line breaks are changed. But, currently the above change
break it.
Yes, it is intended as described in the thread.
https://www.postgresql.org/message-id/20230615.152036.1556630042388070221.horikyota.ntt%40gmail.com
| We could shorten it by removing the "/mnt/server" portion, but
I'm not sure if it's worth doing.
However, I feel it is acceptable to make an exception and exceed 80
characters for this line.
(4)
I found that some binaries, for example ecpg, are not tested with
program_help_ok(). Is it better to add tests in the patch?
Agreed.
BTW, I check the difference with the following commands
# files include "--help"
$ find -name "*.c" | xargs -I {} sh -c 'if [ `grep -e --help {} | wc
-l` -gt 0 ]; then echo {}; fi'
# programs which is tested with program_help_ok
$ find -name "*.pl" | xargs -I {} sh -c 'grep -e program_help_ok {}'
Thanks for sharing your procedure!
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
