Re: [PATCH] Add roman support for to_number function
| От | Hunaid Sohail |
|---|---|
| Тема | Re: [PATCH] Add roman support for to_number function |
| Дата | |
| Msg-id | CAMWA6yZHt7Qjzng03JebNN8VkeVxZTHVrYXrLs27Cn-HrR=AXQ@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [PATCH] Add roman support for to_number function (Tomas Vondra <tomas@vondra.me>) |
| Список | pgsql-hackers |
Hi,
Thanks for reviewing this patch.
On Mon, Dec 9, 2024 at 3:07 AM Tomas Vondra <tomas@vondra.me> wrote:
Thanks for a nice patch. I did a quick review today, seems almost there,
I only have a couple minor comments:
1) Template Patterns for Numeric Formatting
Why the wording change? "input between 1 and 3999" sounds fine to me.
input... seemed to refer to numeric input for to_char roman format. But, after roman support in to_number function shouldn't we modify it? It is the reason I changed it to "valid for numbers 1 to 3999".
2) The docs say "standard numbers" but I'm not sure what that is? We
don't use that term anywhere else, I think. Same for "standard roman
numeral".
I will change "standard numbers" to "numbers".
Note that we are following the standard form. There are other forms too. For example, 4 can be represented as IV (standard) or IIII (non standard).
3) A couple comments need a bit of formatting. Multi-line comments
should start with an empty line, some lines are a bit too long.
I will fix the multi-line formatting.
4) I was wondering if the regression tests check all interesting cases,
and it seems none of the tests hit these two cases:
if (vCount > 1 || lCount > 1 || dCount > 1)
return -1;
SELECT to_number('viv', 'RN') test covers this if statement for the subtraction part. But, I noticed that no test covers simple counts of V, L, D.
I will add SELECT to_number('VV', 'RN') for that.
and
if (!IS_VALID_SUB_COMB(currChar, nextChar))
return -1;
Noted. SELECT to_number('IL', 'RN') test can be added.
Regards,
Hunaid Sohail
В списке pgsql-hackers по дате отправления: