Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-19 Thread Doug Doole
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Applied clean and runs fine. Previous comments were addressed

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-17 Thread Christoph Berg
Re: Peter Eisentraut 2017-08-14 > There are probably a bunch of Perl or Python modules that can be > employed for this. https://github.com/ChristophBerg/postgresql-numeral Christoph -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: ht

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-17 Thread Chris Travers
On Sun, Sep 17, 2017 at 6:43 PM, David Fetter wrote: > On Sat, Sep 16, 2017 at 10:42:49PM +, Douglas Doole wrote: > > Oliver, I took a look at your tests and they look thorough to me. > > > > One recommendation, instead of having 3999 separate selects to test every > > legal roman numeral, wh

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-17 Thread David Fetter
On Sat, Sep 16, 2017 at 10:42:49PM +, Douglas Doole wrote: > Oliver, I took a look at your tests and they look thorough to me. > > One recommendation, instead of having 3999 separate selects to test every > legal roman numeral, why not just do something like this: > > do $$ > declare > i

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-17 Thread Oleg Bartunov
On 3 Aug 2017 16:29, "Oliver Ford" wrote: Adds to the to_number() function the ability to convert Roman numerals to a number. This feature is on the formatting.c TODO list. It is not currently implemented in either Oracle, MSSQL or MySQL so gives PostgreSQL an edge :-) I see use of this in full

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-16 Thread Douglas Doole
I finally figured out why docs weren't building on my machine so I was able to take a look at your doc patch too. I think it's fine. Reading it did suggest a couple extra negative tests to confirm that when 'rn' is specified, all other elements are ignored: select to_number('vii7', 'rn9'); select

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-16 Thread Douglas Doole
Oliver, I took a look at your tests and they look thorough to me. One recommendation, instead of having 3999 separate selects to test every legal roman numeral, why not just do something like this: do $$ declare i int; rn text; rn_val int; begin for i in 1..3999 loop rn :=

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-14 Thread Douglas Doole
Ah. Sorry I missed them - I'll give them a look. (Won't be able to get to it until Saturday though.) On Thu, Sep 14, 2017 at 10:06 PM Oliver Ford wrote: > I'll fix the brace, but there are two other patches in the first email for > tests and docs. For some reason the commitfest app didn't pick th

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-14 Thread Oliver Ford
I'll fix the brace, but there are two other patches in the first email for tests and docs. For some reason the commitfest app didn't pick them up. On Friday, 15 September 2017, Doug Doole wrote: > The following review has been posted through the commitfest application: > make installcheck-world:

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-09-14 Thread Doug Doole
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:not tested Code looks fine, but one niggly complaint at line 146 of the patch fi

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-08-14 Thread Merlin Moncure
On Mon, Aug 14, 2017 at 2:48 PM, Peter Eisentraut wrote: > On 8/3/17 13:45, Robert Haas wrote: >> On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford wrote: >>> Adds to the to_number() function the ability to convert Roman numerals >>> to a number. This feature is on the formatting.c TODO list. It is not

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-08-14 Thread Peter Eisentraut
On 8/3/17 13:45, Robert Haas wrote: > On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford wrote: >> Adds to the to_number() function the ability to convert Roman numerals >> to a number. This feature is on the formatting.c TODO list. It is not >> currently implemented in either Oracle, MSSQL or MySQL so g

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-08-03 Thread Robert Haas
On Thu, Aug 3, 2017 at 2:54 PM, Oliver Ford wrote: > The formatting.c file specifies it as a TODO, so I thought implementing it > would be worthwhile. As there is a to_roman conversion having it the other > way is good for completeness. Huh, didn't know about that. Well, I'm not direly opposed t

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-08-03 Thread Oliver Ford
On Thursday, 3 August 2017, Robert Haas wrote: > On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford > wrote: > > Adds to the to_number() function the ability to convert Roman numerals > > to a number. This feature is on the formatting.c TODO list. It is not > > currently implemented in either Oracle, M

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-08-03 Thread David Fetter
On Thu, Aug 03, 2017 at 01:45:02PM -0400, Robert Haas wrote: > On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford wrote: > > Adds to the to_number() function the ability to convert Roman numerals > > to a number. This feature is on the formatting.c TODO list. It is not > > currently implemented in either

Re: [HACKERS] Add Roman numeral conversion to to_number

2017-08-03 Thread Robert Haas
On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford wrote: > Adds to the to_number() function the ability to convert Roman numerals > to a number. This feature is on the formatting.c TODO list. It is not > currently implemented in either Oracle, MSSQL or MySQL so gives > PostgreSQL an edge :-) I kind of