Re: Allow to_date() and to_timestamp() to accept localized names

On Sat, Jan 11, 2020 at 5:06 PM Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

>
> Thanks. I did a quick review of this patch, and I think it's almost RFC.
>
>
Thanks for reviewing.

> - In func.sgml, it seems we've lost this bit:
>
> <para>
> <literal>TM</literal> does not include trailing blanks.
> <function>to_timestamp</function> and <function>to_date</function>
> ignore
> the <literal>TM</literal> modifier.
> </para>
>
> Does that mean the function no longer ignore the TM modifier? That
> would be somewhat problematic (i.e. it might break some user code).
> But looking at the code I don't see why the patch would have this
> effect, so I suppose it's merely a doc bug.
>
>
It is intentional. This patch uses the TM modifier to identify the usage of
localized names as input for to_timestamp() and to_date().

> - I don't think we need to include examples how to_timestmap ignores
> case, I'd say just stating the fact is clear enough. But if we want to
> have examples, I think we should not inline in the para but use the
> established pattern:
>
> <para>
> Some examples:
> <programlisting>
> ...
> </programlisting>
> </para>
>
> which is used elsewhere in the func.sgml file.
>

I was trying to match the style surrounding the usage notes for date/time
formatting [1]. Agreed that it is not worth an example on its own, so
dropped.

>
> - In formatting.c the "common/unicode_norm.h" should be right after
> includes from "catalog/" to follow the alphabetic order (unless
> there's a reason why that does not work).
>

Fixed.

>
> - I rather dislike the "dim" parameter name, because I immediately think
> "dimension" which makes little sense. I suggest renaming to "nitems"
> or "nelements" or something like that.
>

Agreed, using "nelements" as a better style matchup.

Please, find attached a version addressing the above mentioned.

[1] https://www.postgresql.org/docs/current/functions-formatting.html

Regards,

Juan José Santamaría Flecha

>