review: psql and pset without any arguments

2) there is only one open question /messages/by-id/B6F6FD62F2624C4C9916AC0175D56D880CE00E0E@jenmbs01.ad.intershop.net -
there is no clean relation between output and some pset option.

I don't think so Marc' proposal is ideal (these values are not a variables) - but maybe some enhanced output (only for this resume) can be better:

postgres=# \pset
Output format (format) is aligned.
Border style (border) is 1.

...

+1

Many thanks for taking care of my suggestion.

best regards,

Marc Mamin

Le 07/09/2013 10:02, Pavel Stehule a écrit :

Hello

* patch is cleanly patchable and compilation is without warnings
* all regression tests passed
* no impact on dump, performance or any current features
* no comments to programming style
* we would this feature - it is consistent with \set and it gives a
fast resume about psql printer settings

Issues:
1) it doesn't show linestyle when is default

I looked there and it is probably a small different issue - this value
is initialized as NULL as default. But I dislike a empty output in
this case:

else if (strcmp(param, "linestyle") == 0)
{
if (!popt->topt.line_style)
;
else
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

I propose a verbose result instead nothing:

else if (strcmp(param, "linestyle") == 0)
{
if (!popt->topt.line_style)
printf(_("Line style is unset.\n")) ;
else
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

+1 to show the information even if linestyle is not set but by default
get_line_style() return "ascii" if linestyle is not set. So maybe it is
better to rewrite it as follow:

else if (strcmp(param, "linestyle") == 0)
{
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

This will output:

Line style is ascii.

when linestyle is not set or of course it is set to ascii.

2) there is only one open question
/messages/by-id/B6F6FD62F2624C4C9916AC0175D56D880CE00E0E@jenmbs01.ad.intershop.net
- there is no clean relation between output and some pset option.

I don't think so Marc' proposal is ideal (these values are not a
variables) - but maybe some enhanced output (only for this resume) can
be better:

postgres=# \pset
Output format (format) is aligned.
Border style (border) is 1.
Expanded display (expanded) is off.
Null display (null) is "".
Field separator (fieldsep) is "|".
Tuples only (tuples_only) is off.
Title (title) is unset.
Table attributes (tableattr) unset.
Pager (pager) is used for long output.
Record separator (recordsep) is <newline>.

This expanded output should be used only for this resume (not when a
option was changed or individual ask on option value)

Yes this could be a good accommodation but I really prefer to not
duplicate code and translation between this resume and the output when
these options are set. If we can print the same output messages using:

postgres=# \pset fieldsep '|'
Field separator (fieldsep) is "|".

it could be a good compromise.

Regards,

--
Gilles Darold
Administrateur de bases de données
http://dalibo.com - http://dalibo.org

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2013/9/7 Gilles Darold <gilles.darold@dalibo.com>

Le 07/09/2013 10:02, Pavel Stehule a écrit :

Hello

* patch is cleanly patchable and compilation is without warnings
* all regression tests passed
* no impact on dump, performance or any current features
* no comments to programming style
* we would this feature - it is consistent with \set and it gives a
fast resume about psql printer settings

Issues:
1) it doesn't show linestyle when is default

I looked there and it is probably a small different issue - this value
is initialized as NULL as default. But I dislike a empty output in
this case:

else if (strcmp(param, "linestyle") == 0)
{
if (!popt->topt.line_style)
;
else
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

I propose a verbose result instead nothing:

else if (strcmp(param, "linestyle") == 0)
{
if (!popt->topt.line_style)
printf(_("Line style is unset.\n")) ;
else
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

+1 to show the information even if linestyle is not set but by default
get_line_style() return "ascii" if linestyle is not set. So maybe it is
better to rewrite it as follow:

else if (strcmp(param, "linestyle") == 0)
{
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

This will output:

Line style is ascii.

when linestyle is not set or of course it is set to ascii.

2) there is only one open question

/messages/by-id/B6F6FD62F2624C4C9916AC0175D56D880CE00E0E@jenmbs01.ad.intershop.net

- there is no clean relation between output and some pset option.

I don't think so Marc' proposal is ideal (these values are not a
variables) - but maybe some enhanced output (only for this resume) can
be better:

postgres=# \pset
Output format (format) is aligned.
Border style (border) is 1.
Expanded display (expanded) is off.
Null display (null) is "".
Field separator (fieldsep) is "|".
Tuples only (tuples_only) is off.
Title (title) is unset.
Table attributes (tableattr) unset.
Pager (pager) is used for long output.
Record separator (recordsep) is <newline>.

This expanded output should be used only for this resume (not when a
option was changed or individual ask on option value)

Yes this could be a good accommodation but I really prefer to not
duplicate code and translation between this resume and the output when
these options are set. If we can print the same output messages using:

postgres=# \pset fieldsep '|'
Field separator (fieldsep) is "|".

it could be a good compromise.

ok

Pavel

Le 07/09/2013 21:22, Pavel Stehule a écrit :

2013/9/7 Gilles Darold <gilles.darold@dalibo.com
<mailto:gilles.darold@dalibo.com>>

Le 07/09/2013 10:02, Pavel Stehule a écrit :

Hello

* patch is cleanly patchable and compilation is without warnings
* all regression tests passed
* no impact on dump, performance or any current features
* no comments to programming style
* we would this feature - it is consistent with \set and it gives a
fast resume about psql printer settings

Issues:
1) it doesn't show linestyle when is default

I looked there and it is probably a small different issue - this

value

is initialized as NULL as default. But I dislike a empty output in
this case:

else if (strcmp(param, "linestyle") == 0)
{
if (!popt->topt.line_style)
;
else
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

I propose a verbose result instead nothing:

else if (strcmp(param, "linestyle") == 0)
{
if (!popt->topt.line_style)
printf(_("Line style is unset.\n")) ;
else
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

+1 to show the information even if linestyle is not set but by default
get_line_style() return "ascii" if linestyle is not set. So maybe
it is
better to rewrite it as follow:

else if (strcmp(param, "linestyle") == 0)
{
printf(_("Line style is %s.\n"),
get_line_style(&popt->topt)->name);
}

This will output:

Line style is ascii.

when linestyle is not set or of course it is set to ascii.

2) there is only one open question

/messages/by-id/B6F6FD62F2624C4C9916AC0175D56D880CE00E0E@jenmbs01.ad.intershop.net

- there is no clean relation between output and some pset option.

I don't think so Marc' proposal is ideal (these values are not a
variables) - but maybe some enhanced output (only for this

resume) can

be better:

postgres=# \pset
Output format (format) is aligned.
Border style (border) is 1.
Expanded display (expanded) is off.
Null display (null) is "".
Field separator (fieldsep) is "|".
Tuples only (tuples_only) is off.
Title (title) is unset.
Table attributes (tableattr) unset.
Pager (pager) is used for long output.
Record separator (recordsep) is <newline>.

This expanded output should be used only for this resume (not when a
option was changed or individual ask on option value)

Yes this could be a good accommodation but I really prefer to not
duplicate code and translation between this resume and the output when
these options are set. If we can print the same output messages using:

postgres=# \pset fieldsep '|'
Field separator (fieldsep) is "|".

it could be a good compromise.

ok

Pavel

Hello,

Sorry for the delay, here is the new patch. The \pset output will look
like follow:

postgres=# \pset
Border style (border) is 1.
Target width (columns) unset.
Expanded display (expanded) is off.
Field separator (fieldsep) is "|".
Default footer (footer) is on.
Output format (format) s aligned.
Line style (linestyle) is ascii.
Null display (null) is "".
Locale-adjusted numeric output (numericlocale) is off.
Pager (pager) is used for long output.
Record separator (recordsep) is <newline>.
Table attributes (tableattr) unset.
Title (title) unset.
Tuples only (tuples_only) is off.

postgres=# \pset null #
Null display (null) is "#".
postgres=#

This also mean that all translation strings of those messages should be
updated.

If we don't want to modify those messages, I can provide an other patch
which print output as follow:

postgres=# \pset
border: Border style is 1.
columns: Target width unset.
expanded: Expanded display is off.
fieldsep: Field separator is "|".
footer: Default footer is on.
format: Output format is aligned.
linestyle: Line style is ascii.
null: Null display is "".
numericlocale: Locale-adjusted numeric output is off.
pager: Pager is used for long output.
recordsep: Record separator is <newline>.
tableattr: Table attributes unset.
title: Title unset.
tuples_only: Tuples only is off.

postgres=# \pset null #
Null display is "#".
postgres=#

I think the first output is better but it need translation work. Let me
know.

Regards,

--
Gilles Darold
Administrateur de bases de données
http://dalibo.com - http://dalibo.org

Pavel

Hello,

Sorry for the delay, here is the new patch. The \pset output will look
like follow:

postgres=# \pset

Border style (border) is 1.
Target width (columns) unset.

Expanded display (expanded) is off.
Field separator (fieldsep) is "|".
Default footer (footer) is on.
Output format (format) s aligned.
Line style (linestyle) is ascii.

Null display (null) is "".
Locale-adjusted numeric output (numericlocale) is off.

Pager (pager) is used for long output.
Record separator (recordsep) is <newline>.
Table attributes (tableattr) unset.
Title (title) unset.

Tuples only (tuples_only) is off.

postgres=# \pset null #
Null display (null) is "#".
postgres=#

This also mean that all translation strings of those messages should be
updated.

If we don't want to modify those messages, I can provide an other patch
which print output as follow:

postgres=# \pset
border: Border style is 1.
columns: Target width unset.
expanded: Expanded display is off.
fieldsep: Field separator is "|".
footer: Default footer is on.
format: Output format is aligned.
linestyle: Line style is ascii.
null: Null display is "".
numericlocale: Locale-adjusted numeric output is off.
pager: Pager is used for long output.
recordsep: Record separator is <newline>.
tableattr: Table attributes unset.
title: Title unset.
tuples_only: Tuples only is off.

postgres=# \pset null #
Null display is "#".
postgres=#

I think the first output is better but it need translation work. Let me
know.

I prefer first output too

Regards

Pavel

Gilles Darold escribi�:

+	else if (strcmp(param, "numericlocale") == 0)
+	{
+		if (popt->topt.numericLocale)
+			puts(_("Locale-adjusted numeric output (numericlocale) is on."));
+		else
+			puts(_("Locale-adjusted numeric output (numericlocale) is off."));
+	}

Please don't make the variable name part of the translatable message. I
suggest using the following pattern:

+	else if (strcmp(param, "numericlocale") == 0)
+	{
+		if (popt->topt.numericLocale)
+			printf(_("Locale-adjusted numeric output (%s) is on."), "numericlocale");
+		else
+			printf(_("Locale-adjusted numeric output (%s) is off."), "numericlocale");
+	}

Otherwise it will be too easy for the translator to make the mistake
that the variable name needs translation too.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le 30/09/2013 05:43, Alvaro Herrera a �crit :

Gilles Darold escribi�:

+	else if (strcmp(param, "numericlocale") == 0)
+	{
+		if (popt->topt.numericLocale)
+			puts(_("Locale-adjusted numeric output (numericlocale) is on."));
+		else
+			puts(_("Locale-adjusted numeric output (numericlocale) is off."));
+	}

Please don't make the variable name part of the translatable message. I
suggest using the following pattern:

+	else if (strcmp(param, "numericlocale") == 0)
+	{
+		if (popt->topt.numericLocale)
+			printf(_("Locale-adjusted numeric output (%s) is on."), "numericlocale");
+		else
+			printf(_("Locale-adjusted numeric output (%s) is off."), "numericlocale");
+	}

Otherwise it will be too easy for the translator to make the mistake
that the variable name needs translation too.

That's right, here is the patch modified with just a little change with
your suggestion:

if (popt->topt.numericLocale)
printf(_("Locale-adjusted numeric output (%s) is
on.\n"), param);
else
printf(_("Locale-adjusted numeric output (%s) is
off.\n"), param);

Thanks

--
Gilles Darold
Administrateur de bases de donn�es
http://dalibo.com - http://dalibo.org

Hi

2013/9/30 Gilles Darold <gilles.darold@dalibo.com>:
(...)

That's right, here is the patch modified with just a little change with
your suggestion:

if (popt->topt.numericLocale)
printf(_("Locale-adjusted numeric output (%s) is
on.\n"), param);
else
printf(_("Locale-adjusted numeric output (%s) is
off.\n"), param);

I'm a bit late to this thread, but I'd just like to say I find this a useful
feature which I've missed on the odd occasion.

BTW there is a slight typo in the patch, "s" should be "is":

"Output format (format) s aligned."

+ printf(_("Output format (%s) s %s.\n"), param,
+ _align2string(popt->topt.format));

Regards

Ian Barwick

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Please remove the tabs from the SGML files.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le 30/09/2013 17:35, Peter Eisentraut a �crit :

Please remove the tabs from the SGML files.

Done. I've also fixed the typo reported by Ian. Here is the attached v4
patch.

Thanks a lot for your review.

Regards,

--
Gilles Darold
Administrateur de bases de donn�es
http://dalibo.com - http://dalibo.org

Hello all

I am thinking so almost all is done

I fixed a help and appended a simple test

But it is a cosmetic changes.

Comments?

Regards

Pavel Stehule

2013/9/30 Gilles Darold <gilles.darold@dalibo.com>

Hello all

there are no comments, so I'll close this topic

This feature is ready for commit

Regards

Pavel Stehule

2013/10/1 Pavel Stehule <pavel.stehule@gmail.com>

On Wed, Oct 2, 2013 at 5:18 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hello all

there are no comments, so I'll close this topic

This feature is ready for commit

The patch looks nice and clean, and I like the feature, too. Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers