Re: Make --help output fit within 80 columns per line

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