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

On Mon, Aug 21, 2023 at 1:09 PM Masahiro Ikeda 
<ikedamsh@oss.nttdata.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