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

On Mon, Aug 21, 2023 at 1:09 PM Masahiro Ikeda
<ikedamsh(at)oss(dot)nttdata(dot)com> wrote:
> (1)
> Why don't you add test for the purpose? It could be overkill...
> I though the following function is the best place.

Added the test.

BTW, psql --help outputs the content of PGHOST, which caused a failure
in the test:

```
-h, --host=HOSTNAME database server host or socket directory
(default:
"/var/folders/m7/9snkd5b54cx_b4lxkl9ljlcc0000gn/T/LobrmSUf7t")
```

It may be overkill, added a logic for removing the content of PGHOST.

On 2023-08-23 09:45, Masahiro Ikeda wrote:
> Hi,
>
> On 2023-08-22 22:57, torikoshia wrote:
>> On 2023-08-21 13:08, Masahiro Ikeda wrote:
>>> (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).
>
> OK. Sorry, I missed the sentence above.
> I'd like to hear what others comment too.

Although there are no comments, attached patch modifies vaccumlo.

>>> (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.
>
> Thanks for sharing the thread. I understood.
>
> It seems that the directory name should be consistent with the example
> of archive_cleanup_command. However, it does not seem appropriate to
> make archive_cleanup_command to use a relative path.
>
> ```
>> pg_archivecleanup --help
> (snip)
> e.g.
> archive_cleanup_command = 'pg_archivecleanup /mnt/server/archiverdir
> %r'
>
> Or for use as a standalone archive cleaner:
> e.g.
> pg_archivecleanup /mnt/server/archiverdir
> 000000010000000000000010.00000020.backup
> ```
>
> IMHO, is simply breaking the line acceptable?

Agreed.

> (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?

Added program_help_ok() to ecpg and pgbench.
Although pg_regress and zic are not tested using program_help_ok, but
left as they are because they are not commands that users execute
directly.

--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation