From: | Masahiro Ikeda <ikedamsh(at)oss(dot)nttdata(dot)com> |
---|---|
To: | torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> |
Cc: | Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Make --help output fit within 80 columns per line |
Date: | 2023-08-21 04:08:58 |
Message-ID: | 3965013067e7b6195132da6c0b80a609@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2023-07-05 10:47, torikoshia wrote:
> Hi,
>
> As discussed in [1], outputs of --help for some commands fits into 80
> columns
> per line, while others do not.
>
> Since it seems preferable to have consistent line break policy and some
> people
> use 80-column terminal, wouldn't it be better to make all commands in
> 80
> columns per line?
>
> Attached patch which does this for src/bin commands.
>
> If this is the way to go, I'll do same things for contrib commands.
>
> [1]
> https://www.postgresql.org/message-id/3fe4af5a0a81fc6a2ec01cb484c0a487%40oss.nttdata.com
Thanks for making the patches! I have some comments to v1 patch.
(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;
}
(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
(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.
(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?
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 {}'
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
From | Date | Subject | |
---|---|---|---|
Next Message | Masahiro Ikeda | 2023-08-21 04:32:24 | Re: Fix pg_stat_reset_single_table_counters function |
Previous Message | Richard Guo | 2023-08-21 03:48:07 | Support run-time partition pruning for hash join |