On Fri, Aug 21, 2020 at 5:21 AM Pavel Stehule <pavel.steh...@gmail.com> wrote:
> new patch attached Thanks for taking some of my previous review comments. I have re-checked the string_to_table_20200820.patch. Below are some remaining questions/comments: ==== COMMENT (help text) + Splits the <parameter>string</parameter> at occurrences + of <parameter>delimiter</parameter> and forms the remaining data + into a <type>text</type> tavke. What did you mean by "remaining" in that description? It gets a bit strange thinking about remaining NULLs, or remaining empty strings. Why not just say "... and forms the data into a <type>text</type> table." --- + Splits the <parameter>string</parameter> at occurrences + of <parameter>delimiter</parameter> and forms the remaining data + into a <type>text</type> tavke. Typo: "tavke." -> "table." ==== COMMENT (help text reference to regexp_split_to_table) + input <parameter>string</parameter> can be done by function + <function>regexp_split_to_table</function> (see <xref linkend="functions-posix-regexp"/>). + </para> In the previous review I suggested adding a reference to the regexp_split_to_table function. A hyperlink would be a bonus, but maybe it is not possible. The hyperlink added in the latest patch is to page for POSIX Regular Expressions, which doesn't seem appropriate. ==== QUESTION (test cases) Thanks for merging lots of my additional test cases! Actually, the previous PDF I sent was 2 pages long but you only merged the tests of page 1. I wondered was it accidental to omit all those 2nd page tests? ==== QUESTION (function name?) I noticed that ALL current string functions that use delimiters have the word "split" in their name. e.g. * regexp_split_to_array * regexp_split_to_table * split_part But "string_to_table" is not following this pattern. Maybe a different choice of function name would be more consistent with what is already there? e.g. split_to_table, string_split_to_table, etc. ==== Kind Regards, Peter Smith. Fujitsu Australia
