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

[torikoshia]

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

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

[Peter Eisentraut]

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.

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

[torikoshia]

On 2023-09-25 15:27, torikoshia wrote:
On Tue, Sep 19, 2023 at 3:23 AM Greg Sabino Mullane 
 wrote:

Thanks for your investigation!

On Fri, Sep 15, 2023 at 11:11 AM torikoshia 
 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

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

[torikoshia]

On Tue, Sep 19, 2023 at 3:23 AM Greg Sabino Mullane  
wrote:

Thanks for your investigation!

On Fri, Sep 15, 2023 at 11:11 AM torikoshia 
 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_b4lxkl9ljlccgn/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 CorporationFrom b0ae0826374ce86c95ee36637913b65f865d6e0b Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Mon, 25 Sep 2023 10:40:36 +0900
Subject: [PATCH v1 1/2] Added a test for checking the line length of --help
 output.

---
 src/test/perl/PostgreSQL/Test/Utils.pm | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index d66fe1cf45..291b5dcbbb 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -884,6 +884,14 @@ 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");
+
+	# We set the hard limit on the length of line to 120
+	subtest "$cmd --help outputs fit within 120 columns per line" => sub {
+	foreach my $line (split /\n/, $stdout)
+	{
+		ok(length($line) <= 120, "$line");
+	}
+};
 	return;
 }
 
-- 
2.39.2

From 207c4059b42e975bc788452838099da825972d15 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Mon, 25 Sep 2023 10:52:08 +0900
Subject: [PATCH v1 2/2] Removed environment-variable-based defaults in psql --help

---
 src/bin/psql/help.c | 33 -
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 12280c0e54..3b2d59e2ee 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -50,22 +50,10 @@
 void
 usage(unsigned short int pager)
 {
-	const char *env;
-	const char *user;
-	char	   *errstr;
 	PQExpBufferData buf;
 	int			nlcount;
 	FILE	   *output;
 
-	/* Find default user, in case we need it. */
-	user = getenv("PGUSER");
-	if (!user)
-	{
-		user = get_user_name();
-		if (!user)
-			pg_fatal("%s", errstr);
-	}
-
 	/*
 	 * To avoid counting the output lines manually, build the output in "buf"
 	 * and then count them.
@@ -77,13 +65,8 @@ usage(unsigned short int pager)
 	HELP0("  psql [OPTION]... [DBNAME [USERNAME]]\n\n");
 
 	HELP0("General options:\n");
-	/* Display default database */
-	env = getenv("PGDATABASE");
-	if (!env)
-		env = user;
 	HELP0("  -c, --command=COMMANDrun only single command (SQL or internal) and exit\n");
-	HELPN("  -d, --dbname=DBNAME  database name to connect to (default: \"%s\")\n",
-		  env);
+	HELP0("  -d, --dbname=DBNAME  database name to connect to\n");
 	HELP0("  -f, --file=FILENAME  execute commands from file, then exit\n");
 	HELP0("  -l, --list   list available databases, then exit\n");
 	HELP0("  -v, --set=, --variable=NAME=VALUE\n"
@@ -128,17 +111,9 @@ 

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

[Peter Eisentraut]

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_b4lxkl9ljlccgn/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.

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

[Greg Sabino Mullane]

On Fri, Sep 15, 2023 at 11:11 AM torikoshia 
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.

See also: https://hilton.org.uk/blog/source-code-line-length

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. :)

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

[torikoshia]

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 
.
[23:11:10.382](0.000s) ok 21 - PostgreSQL home page: 


[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 CorporationFrom 3dbfdb79680a0c1b68d4f742ae408810a1ee999d Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Fri, 15 Sep 2023 23:29:23 +0900
Subject: [PATCH v1] Added a test for checking the length of --help output

---
 src/test/perl/PostgreSQL/Test/Utils.pm | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f..989f369ae7 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -843,6 +843,13 @@ 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");
+
+	subtest "$cmd --help outputs fit within 80 columns per line" => sub {
+	foreach my $line (split /\n/, $stdout)
+	{
+		ok(length($line) <= 80, "$line");
+	}
+};
 	return;
 }
 
-- 
2.39.2

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

[Greg Sabino Mullane]

On Tue, Jul 4, 2023 at 9:47 PM torikoshia 
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

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

[torikoshia]

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

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

[Peter Eisentraut]

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.

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

[torikoshia]

On Mon, Aug 21, 2023 at 1:09 PM Masahiro Ikeda 
 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_b4lxkl9ljlccgn/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
00010010.0020.backup\n"));
+"  pg_archivecleanup archiverdir
00010010.0020.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
00010010.0020.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 CorporationFrom 4728cab3e39f7994b8b6dd41787cac90b5cb64f6 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Thu, 31 Aug 2023 16:22:39 +0900
Subject: [PATCH v2] Make --help output fit within 80 columns per line

Outputs of --help output of some commands fit into 80 columns per line,
while others do not.
For the consistency and for 80-column terminal, this patch makes them
fit within 80 columns per line.

Also this patch adds a test which checks if --help outputs fit within
80 columns per line.

---
 contrib/vacuumlo/vacuumlo.c   |  3 ++-
 src/bin/initdb/initdb.c   |  6 --
 src/bin/pg_amcheck/pg_amcheck.c   | 21 ---
 src/bin/pg_archivecleanup/pg_archivecleanup.c |  3 ++-
 src/bin/pg_basebackup/pg_receivewal.c | 12 +++
 src/bin/pg_basebackup/pg_recvlogical.c| 18 ++--
 src/bin/pg_checksums/pg_checksums.c   |  3 ++-
 src/bin/pg_dump/pg_dump.c |  3 ++-
 src/bin/pg_dump/pg_dumpall.c  |  3 ++-
 src/bin/pg_dump/pg_restore.c  |  6 --
 src/bin/pg_upgrade/option.c   |  9 +---
 src/bin/pgbench/pgbench.c |  6 --
 src/bin/pgbench/t/002_pgbench_no_server.pl|  1 +
 src/bin/psql/help.c   |  9 +---
 src/bin/scripts/createdb.c|  3 ++-
 src/bin/scripts/pg_isready.c  |  3 ++-
 src/bin/scripts/vacuumdb.c| 21 ---
 src/interfaces/ecpg/Makefile  |  1 +
 

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

[Masahiro Ikeda]

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
00010010.0020.backup\n"));
+"  pg_archivecleanup archiverdir
00010010.0020.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 
00010010.0020.backup

```

IMHO, is simply breaking the line acceptable?

```
Or for use as a standalone archive cleaner:
e.g.
  pg_archivecleanup /mnt/server/archiverdir
  00010010.0020.backup
```

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

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

[torikoshia]

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
00010010.0020.backup\n"));
+"  pg_archivecleanup archiverdir
00010010.0020.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

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

[Masahiro Ikeda]

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 
00010010.0020.backup\n"));
+			 "  pg_archivecleanup archiverdir 
00010010.0020.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