Обсуждение: Make --help output fit within 80 columns per line
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 -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
Вложения
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
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
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. >> (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? ``` Or for use as a standalone archive cleaner: e.g. pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup ``` Regards, -- Masahiro Ikeda NTT DATA CORPORATION
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
Вложения
I like this work a lot. It's good to give developers easy to verify guidance about formatting the --help messages. However, I think instead of just adding a bunch of line breaks to satisfy the test, we should really try to make the lines shorter by rewording. Otherwise, chances are we are making the usability worse for many users, because it's more likely that part of the help will scroll off the screen. For example, in many cases, we could replace "do not" by "don't", or we could decrease the indentation of the second column by a few spaces, or just reword altogether. Also, it would be very useful if the TAP test function could print out the violating lines if a test fails. (Similar to how is() and like() print the failing values.) Maybe start with that, and then it's easier to play with different wording variants to try to make it fit better.
On 2023-09-12 15:27, Peter Eisentraut wrote: > I like this work a lot. It's good to give developers easy to verify > guidance about formatting the --help messages. > > However, I think instead of just adding a bunch of line breaks to > satisfy the test, we should really try to make the lines shorter by > rewording. Otherwise, chances are we are making the usability worse > for many users, because it's more likely that part of the help will > scroll off the screen. For example, in many cases, we could replace > "do not" by "don't", or we could decrease the indentation of the > second column by a few spaces, or just reword altogether. > > Also, it would be very useful if the TAP test function could print out > the violating lines if a test fails. (Similar to how is() and like() > print the failing values.) Maybe start with that, and then it's > easier to play with different wording variants to try to make it fit > better. Thanks for the review! I'll try to fix the patch according to your comments. -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
On Tue, Jul 4, 2023 at 9:47 PM torikoshia <torikoshia@oss.nttdata.com> wrote:
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?
All this seems an awful lot of work to support this mythical 80-column terminal user. It's 2023, perhaps it's time to widen the default assumption past 80 characters?
Cheers,
Greg
On 2023-09-12 15:27, Peter Eisentraut wrote:
> Also, it would be very useful if the TAP test function could print out
> the violating lines if a test fails. (Similar to how is() and like()
> print the failing values.)
Attached patch for this.
Below are the the outputs when test failed:
```
$ cd contrib/vacuumlo
$ make check
...(snip)...
t/001_basic.pl .. 1/?
# Failed test ' -n, --dry-run don't remove large
objects, just show what would be done'
# at
/home/atorik/postgres/contrib/vacuumlo/../../src/test/perl/PostgreSQL/Test/Utils.pm
line 850.
# Looks like you failed 1 test of 21.
# Failed test 'vacuumlo --help outputs fit within 80 columns per line'
# at t/001_basic.pl line 10.
# Looks like you failed 1 test of 9.
t/001_basic.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/9 subtests
Test Summary Report
-------------------
t/001_basic.pl (Wstat: 256 (exited 1) Tests: 9 Failed: 1)
Failed test: 4
Non-zero exit status: 1
Files=1, Tests=9, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.04 cusr
0.01 csys = 0.07 CPU)
Result: FAIL
```
```
$ cat tmp_check/log/regress_log_001_basic
# Running: vacuumlo --help
[23:11:10.378](0.230s) ok 1 - vacuumlo --help exit code 0
[23:11:10.379](0.001s) ok 2 - vacuumlo --help goes to stdout
[23:11:10.379](0.000s) ok 3 - vacuumlo --help nothing to stderr
[23:11:10.380](0.000s) # Subtest: vacuumlo --help outputs fit within 80
columns per line
[23:11:10.380](0.001s) ok 1 - vacuumlo removes unreferenced large
objects from databases.
[23:11:10.380](0.000s) ok 2 -
[23:11:10.381](0.000s) ok 3 - Usage:
[23:11:10.381](0.000s) ok 4 - vacuumlo [OPTION]... DBNAME...
[23:11:10.381](0.000s) ok 5 -
[23:11:10.381](0.000s) ok 6 - Options:
[23:11:10.381](0.000s) ok 7 - -l, --limit=LIMIT commit
after removing each LIMIT large objects
[23:11:10.382](0.000s) ok 20 - Report bugs to
<pgsql-bugs@lists.postgresql.org>.
[23:11:10.382](0.000s) ok 21 - PostgreSQL home page:
<https://www.postgresql.org/>
[23:11:10.382](0.000s) 1..21
[23:11:10.382](0.000s) # Looks like you failed 1 test of 21.
[23:11:10.382](0.000s) not ok 4 - vacuumlo --help outputs fit within 80
columns per line
[23:11:10.382](0.000s)
[23:11:10.382](0.000s) # Failed test 'vacuumlo --help outputs fit
within 80 columns per line'
# at t/001_basic.pl line 10.
# Running: vacuumlo --version
[23:11:10.388](0.005s) ok 5 - vacuumlo --version exit code 0
[23:11:10.388](0.000s) ok 6 - vacuumlo --version goes to stdout
[23:11:10.388](0.000s) ok 7 - vacuumlo --version nothing to stderr
# Running: vacuumlo --not-a-valid-option
[23:11:10.391](0.003s) ok 8 - vacuumlo with invalid option nonzero exit
code
[23:11:10.391](0.000s) ok 9 - vacuumlo with invalid option prints error
message
[23:11:10.391](0.000s) 1..9
[23:11:10.391](0.000s) # Looks like you failed 1 test of 9.
```
I feel using subtest in Test::More improves readability.
On 2023-09-14 02:46, Greg Sabino Mullane wrote:
> All this seems an awful lot of work to support this mythical 80-column
> terminal user.
> It's 2023, perhaps it's time to widen the default assumption past 80
> characters?
That may be a good idea.
However, from what I have seen some basic commands like `ls` in my Linux
environments, the man command has over 100 characters per line, while
the output of the --help option seems to be within 80 characters per
line.
Also, the current PostgreSQL commands follow the "no more than 80
characters per line".
I do not intend to adhere to this rule(my terminals are usually bigger
than 80 chars per line), but wouldn't it be a not bad direction to use
80 characters for all commands?
Thoughts?
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Вложения
On Fri, Sep 15, 2023 at 11:11 AM torikoshia <torikoshia@oss.nttdata.com> wrote:
I do not intend to adhere to this rule(my terminals are usually bigger
than 80 chars per line), but wouldn't it be a not bad direction to use
80 characters for all commands?
Well, that's the question du jour, isn't it? The 80 character limit is based on punch cards, and really has no place in modern systems. While gnu systems are stuck in the past, many other ones have moved on to more sensible defaults:
$ wget --help | wc -L
110
110
$ gcloud --help | wc -L
122
122
$ yum --help | wc -L
122
122
git is an interesting one, as they force things through a pager for their help, but if you look at their raw help text files, they have plenty of times they go past 80 when needed:
$ wc -L git/Documentation/git-*.txt | sort -g | tail -20
109 git-filter-branch.txt
109 git-rebase.txt
116 git-diff-index.txt
116 git-http-fetch.txt
117 git-restore.txt
122 git-checkout.txt
122 git-ls-tree.txt
129 git-init-db.txt
131 git-push.txt
132 git-update-ref.txt
142 git-maintenance.txt
144 git-interpret-trailers.txt
146 git-cat-file.txt
148 git-repack.txt
161 git-config.txt
162 git-notes.txt
205 git-stash.txt
251 git-submodule.txt
109 git-rebase.txt
116 git-diff-index.txt
116 git-http-fetch.txt
117 git-restore.txt
122 git-checkout.txt
122 git-ls-tree.txt
129 git-init-db.txt
131 git-push.txt
132 git-update-ref.txt
142 git-maintenance.txt
144 git-interpret-trailers.txt
146 git-cat-file.txt
148 git-repack.txt
161 git-config.txt
162 git-notes.txt
205 git-stash.txt
251 git-submodule.txt
So in summary, I think 80 is a decent soft limit, but let's not stress out about some lines going over that, and make a hard limit of perhaps 120.
Cheers,
Greg
P.S. I know this won't change anything right away, but it will get the conversation started, so we can escape the inertia of punch cards / VT100 terminals someday. :)
On 31.08.23 09:47, torikoshia wrote: > 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. I wonder, should we remove this? We display the environment-variable-based defaults in psql --help, but not for any other programs. This is potentially misleading.
On Tue, Sep 19, 2023 at 3:23 AM Greg Sabino Mullane <htamfids@gmail.com> wrote: Thanks for your investigation! > On Fri, Sep 15, 2023 at 11:11 AM torikoshia > <torikoshia@oss.nttdata.com> wrote: >> I do not intend to adhere to this rule(my terminals are usually bigger >> than 80 chars per line), but wouldn't it be a not bad direction to use >> 80 characters for all commands? > > Well, that's the question du jour, isn't it? The 80 character limit is > based on punch cards, and really has no place in modern systems. While > gnu systems are stuck in the past, many other ones have moved on to > more sensible defaults: > > $ wget --help | wc -L > 110 > > $ gcloud --help | wc -L > 122 > > $ yum --help | wc -L > 122 > > git is an interesting one, as they force things through a pager for > their help, but if you look at their raw help text files, they have > plenty of times they go past 80 when needed: > > $ wc -L git/Documentation/git-*.txt | sort -g | tail -20 > 109 git-filter-branch.txt > 109 git-rebase.txt > 116 git-diff-index.txt > 116 git-http-fetch.txt > 117 git-restore.txt > 122 git-checkout.txt > 122 git-ls-tree.txt > 129 git-init-db.txt > 131 git-push.txt > 132 git-update-ref.txt > 142 git-maintenance.txt > 144 git-interpret-trailers.txt > 146 git-cat-file.txt > 148 git-repack.txt > 161 git-config.txt > 162 git-notes.txt > 205 git-stash.txt > 251 git-submodule.txt > > So in summary, I think 80 is a decent soft limit, but let's not stress > out about some lines going over that, and make a hard limit of perhaps > 120. +1. It may be a good compromise. For enforcing the hard limit, is it better to add a regression test like patch 0001? On 2023-09-21 16:45, Peter Eisentraut wrote: > On 31.08.23 09:47, torikoshia wrote: >> 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. > > I wonder, should we remove this? We display the > environment-variable-based defaults in psql --help, but not for any > other programs. This is potentially misleading. Agreed. It seems inconsistent with other commands. Patch 0002 removed environment-variable-based defaults in psql --help. -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
Вложения
On 2023-09-25 15:27, torikoshia wrote: > On Tue, Sep 19, 2023 at 3:23 AM Greg Sabino Mullane > <htamfids@gmail.com> wrote: > Thanks for your investigation! > >> On Fri, Sep 15, 2023 at 11:11 AM torikoshia >> <torikoshia@oss.nttdata.com> wrote: >>> I do not intend to adhere to this rule(my terminals are usually >>> bigger >>> than 80 chars per line), but wouldn't it be a not bad direction to >>> use >>> 80 characters for all commands? >> >> Well, that's the question du jour, isn't it? The 80 character limit is >> based on punch cards, and really has no place in modern systems. While >> gnu systems are stuck in the past, many other ones have moved on to >> more sensible defaults: >> >> $ wget --help | wc -L >> 110 >> >> $ gcloud --help | wc -L >> 122 >> >> $ yum --help | wc -L >> 122 >> >> git is an interesting one, as they force things through a pager for >> their help, but if you look at their raw help text files, they have >> plenty of times they go past 80 when needed: >> >> $ wc -L git/Documentation/git-*.txt | sort -g | tail -20 >> 109 git-filter-branch.txt >> 109 git-rebase.txt >> 116 git-diff-index.txt >> 116 git-http-fetch.txt >> 117 git-restore.txt >> 122 git-checkout.txt >> 122 git-ls-tree.txt >> 129 git-init-db.txt >> 131 git-push.txt >> 132 git-update-ref.txt >> 142 git-maintenance.txt >> 144 git-interpret-trailers.txt >> 146 git-cat-file.txt >> 148 git-repack.txt >> 161 git-config.txt >> 162 git-notes.txt >> 205 git-stash.txt >> 251 git-submodule.txt >> >> So in summary, I think 80 is a decent soft limit, but let's not stress >> out about some lines going over that, and make a hard limit of perhaps >> 120. > > +1. It may be a good compromise. > For enforcing the hard limit, is it better to add a regression test > like patch 0001? Ugh, regression tests failed and it appears to be due to reasons related to meson. I'm going to investigate it. -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
On 25.09.23 08:27, torikoshia wrote: >> So in summary, I think 80 is a decent soft limit, but let's not stress >> out about some lines going over that, and make a hard limit of perhaps >> 120. > > +1. It may be a good compromise. > For enforcing the hard limit, is it better to add a regression test like > patch 0001? > Agreed. It seems inconsistent with other commands. > Patch 0002 removed environment-variable-based defaults in psql --help. I have committed 0002 and a different implementation of 0001. I set the maximum line length to 95, which is the current maximum in use. I'm open to discussing other line lengths, but 1) If we make it longer, then we should also adjust the existing wrapping so that we don't have a mix of lines wrapped at 80 and some significantly longer lines. 2) There are some general readability guidelines that suggest like 66 or 72 characters per line. If you take that and add the option name itself and some indentation, then around 90 does seem like a sensible limit. 3) The examples from other tools posted earlier don't convince me. Some of their --help outputs look like junk and poorly taken care of. So I think nudging people to aim for 80..95 seems like a good target right now. But I'm not against adjustments.
On 2023-09-25 15:27, torikoshia wrote: > Ugh, regression tests failed and it appears to be due to reasons > related to meson. > I'm going to investigate it. ISTM On 2023-10-06 19:49, Peter Eisentraut wrote: > On 25.09.23 08:27, torikoshia wrote: >>> So in summary, I think 80 is a decent soft limit, but let's not >>> stress out about some lines going over that, and make a hard limit of >>> perhaps 120. >> >> +1. It may be a good compromise. >> For enforcing the hard limit, is it better to add a regression test >> like patch 0001? > >> Agreed. It seems inconsistent with other commands. >> Patch 0002 removed environment-variable-based defaults in psql --help. > > I have committed 0002 and a different implementation of 0001. I set > the maximum line length to 95, which is the current maximum in use. Thanks! BTW as far as I investigated, the original 0002 patch failed because current meson doesn't accept subtest outputs. As I commented below thread a few days ago, they once modified to accept subtest outputs, but not anymore. https://github.com/mesonbuild/meson/issues/10032 > I'm open to discussing other line lengths, but > > 1) If we make it longer, then we should also adjust the existing > wrapping so that we don't have a mix of lines wrapped at 80 and some > significantly longer lines. > > 2) There are some general readability guidelines that suggest like 66 > or 72 characters per line. If you take that and add the option name > itself and some indentation, then around 90 does seem like a sensible > limit. > > 3) The examples from other tools posted earlier don't convince me. > Some of their --help outputs look like junk and poorly taken care of. > > So I think nudging people to aim for 80..95 seems like a good target > right now. But I'm not against adjustments. -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation