Re: Please Review fix for reduced value parser 8024076

2013-10-02 Thread Stephen Colebourne
I suggest taking the patch as is and raising a new lower priority bug for the effective chrono aspect. Stephen On 1 October 2013 19:51, roger riggs roger.ri...@oracle.com wrote: Hi Stephen, The proposed approach makes sense to me, shall I take the patch as is or wait to integrate until for

Please Review fix for reduced value parser 8024076

2013-10-02 Thread roger riggs
Please review this fix for parsing two digit years in an Chronology. The webrev includes Stephen's proposed alternate method that provides a ChronoLocalDate as the base date. http://cr.openjdk.java.net/~rriggs/webrev-two-digit-8024076/ Thanks, Roger p.s. the design issue raised in the

Re: Please Review fix for reduced value parser 8024076

2013-10-02 Thread Stephen Colebourne
My patch still looks fine to me :-) Stephen On 2 October 2013 16:19, roger riggs roger.ri...@oracle.com wrote: Please review this fix for parsing two digit years in an Chronology. The webrev includes Stephen's proposed alternate method that provides a ChronoLocalDate as the base date.

Re: Please Review fix for reduced value parser 8024076

2013-10-02 Thread Xueming Shen
Should move the static field BASE_DATE into ReducePrinterParser? Logically (and for performance, if it matters at all) RPP appears to be a better place for this constant. The rest looks fine. -Sherman On 10/02/2013 08:19 AM, roger riggs wrote: Please review this fix for parsing two digit

Re: Please Review fix for reduced value parser 8024076

2013-10-02 Thread Xueming Shen
On 10/02/2013 10:20 AM, roger riggs wrote: Hi Sherman, The BASE_DATE is the default ChronoLocalDate and is used outside of RPP. RPP itself uses any ChronoLocalDate, not the specific one. Scoping BASE_DATE to RPP would not delay the initialization since it would need to be initialized at line

Re: Please Review fix for reduced value parser 8024076

2013-10-02 Thread roger riggs
Hi Sherman, Thanks for the detailed review, the webrev has been updated with your recommendation. Roger On 10/2/2013 1:50 PM, Xueming Shen wrote: On 10/02/2013 10:20 AM, roger riggs wrote: Hi Sherman, The BASE_DATE is the default ChronoLocalDate and is used outside of RPP. RPP itself uses

Re: Please Review fix for reduced value parser 8024076

2013-10-01 Thread roger riggs
Hi Stephen, The proposed approach makes sense to me, shall I take the patch as is or wait to integrate until for the mentioned update of effective chrono? Thanks, Roger On 9/22/2013 10:27 AM, Stephen Colebourne wrote: The patch only changes the text of one of the two appendValueReduced

Re: Please Review fix for reduced value parser 8024076

2013-09-22 Thread Stephen Colebourne
The patch only changes the text of one of the two appendValueReduced methods. The patch does not handle week based years or provide for users to add their own year fields. It also does not handle formatting. After much thinking, I think the right solution is to add a new appendValueReduced method

Please Review fix for reduced value parser 8024076

2013-09-21 Thread roger riggs
Hi, The java.time reduced value parser does work as expected (issue 8024076) for chronologies other than ISO. The base value is assumed to be chronology independent but is not converted to the requested Chronology before it is used. Please review: