On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber <phil@omniti.com> wrote:
>>
>> Here is the updated patch. I renamed it, but using v5 to stay consistent.
>
>
> After looking at this patch, I found the following problems:
> - There are a couple of whitespaces still in the code, particularly at the
> end of those lines
> + const char *pguser = NULL;
> + const char *pgdbname = NULL;
Mystery how those got in there. Fixed.
> - When describing the values that are returned by pg_isready, it is awkward
> to refer to the returning values as plain integers as those values are part
> of an enum. You should add references to PQPING_OK, PQPING_REJECT,
> PQPING_NO_RESPONSE and PQPING_NO_ATTEMPT instead. Also add to reference
> links in the docs redirecting to them, with things of the type <xref
> linkend="libpq-pqpingparams-pqping-ok"> or related.
Fixed. I changed the wording a little too.
> - Same thing with this example:
> + <para>
> + Standard Usage:
> + <screen>
> + <prompt>$</prompt> <userinput>pg_isready</userinput>
> + <prompt>$</prompt> <userinput>echo $?</userinput>
> + <computeroutput>0</computeroutput>
> + </screen>
> + </para>
> For the time being PQPING_OK returns 0 because it is on top of the enum
> PGPing, but this might change if for a reason or another the order of
> outputs is changed.
So I understand what you mean by the ordering might change, but this
is actual output from the shell. I'm not sure how to convey that
sentiment properly here and still have a real example. Perhaps just
remove the example?
>
> Once those things are fixed, I think this will be ready for committer review
> as everybody here seem to agree with your approach.
>
> --
> Michael Paquier
> http://michael.otacoo.com