On Sat, Jan 11, 2020 at 5:06 PM Tomas Vondra <tomas.von...@2ndquadrant.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 >
0001-Allow-localized-month-names-to_date-v5.patch
Description: Binary data
