Re: proposal - function string_to_table

pá 21. 8. 2020 v 11:08 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> pá 21. 8. 2020 v 9:44 odesílatel Peter Smith <smithpb2250(at)gmail(dot)com>
> napsal:
>
>> On Fri, Aug 21, 2020 at 5:21 AM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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."
>>
>
> This text is taken from doc for string_to_array
>

I fixed typo. I hope and expect so doc will be finalized by native
speakers.

>
>
>> ====
>>
>> 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.
>>
>
> ok I remove it
>
>>
>> ====
>>
>> 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?
>>
>
> I'll check it
>

I forgot it - now it is merged. Maybe it is over dimensioned for one
function, but it is (at the end) a test of string_to_array function too.

>
>> ====
>>
>> 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.
>>
>
> I don't agree. This function is twin (with almost identical behaviour) for
> "string_to_array" function, so I think so the name is correct.
>

Unfortunately - there is not consistency in naming already, But I think so
string_to_table is a better name, because this function is almost identical
with string_to_array.

Regards

Pavel

>
>> ====
>>
>> Kind Regards,
>> Peter Smith.
>> Fujitsu Australia
>>
>