On 10.02.23 02:10, Peter Smith wrote:
> On Thu, Feb 9, 2023 at 7:17 PM Jim Jones <jim.jones@uni-muenster.de> wrote:
> 1.
> FYI, there are some whitespace warnings applying the v5 patch
>
Trailing whitespaces removed. The patch applies now without warnings.
> ======
> src/test/regress/sql/xml.sql
>
> 2.
> The v5 patch was already testing NULL, but it might be good to add
> more tests to verify the function is behaving how you want for edge
> cases. For example,
>
> +-- XML pretty print: NULL, empty string, spaces only...
> SELECT xmlpretty(NULL);
> SELECT xmlpretty('');
> SELECT xmlpretty(' ');
Test cases added.
> 3.
> The function is returning XML anyway, so is the '::xml' casting in
> these tests necessary?
>
> e.g.
> SELECT xmlpretty(NULL)::xml; --> SELECT xmlpretty(NULL);
It is indeed not necessary. Most likely I used it for testing and forgot
to remove it afterwards. Now removed.
> ======
> src/include/catalog/pg_proc.dat
>
> 4.
>
> + { oid => '4642', descr => 'Indented text from xml',
> + proname => 'xmlpretty', prorettype => 'xml',
> + proargtypes => 'xml', prosrc => 'xmlpretty' },
>
> Spurious leading space for this new entry.
Removed.
>
> ======
> doc/src/sgml/func.sgml
>
> 5.
> + <foo id="x">
> + <bar id="y">
> + <var id="z">42</var>
> + </bar>
> + </foo>
> +
> +(1 row)
> +
> +]]></screen>
>
> A spurious blank line in the example after the "(1 row)"
Removed.
> ~~~
>
> 6.
> Does this function docs belong in section 9.15.1 "Producing XML
> Content"? Or (since it is not really producing content) should it be
> moved to the 9.15.3 "Processing XML" section?
Moved to the section 9.15.3
Following the suggestion of Peter Eisentraut I renamed the function to
xmlformat().
v6 attached.
Thanks a lot for the review.
Best, Jim
