[threeten-dev] Review for parsing 2 to 4 years #218

[roger riggs]

Thanks  for the detailed review.

Updated Webrev:
     http://cr.openjdk.java.net/~rriggs/webrev-lenient-parse-218/

Javadoc:
     http://cr.openjdk.java.net/~rriggs/javadoc-lenient-parse-218/


On 4/30/2013 2:24 AM, Stephen Colebourne wrote:
> I think the core logic here is now right. In particular its really
> good that there is no difference internally in appendValue() with
> fixed/non-fixed width.
>
> DTFB line 559 needs tweaking to make sense:
> "If there is a value PrinterParser is not active then the
> PrinterParser becomes the active PP;"
fixed
>
> DTFB lines 1370 (Javadoc) and line 1658 (code) differ.
>
> The Javadoc for the two appendValueReduced methods doesn't make it
> clear enough to me as to their difference. It seems to me that the
> simpler method is useful for all normal cases. In other places I've
> got a paragraph for formatting, then a paragraph for parsing - that
> pattern would be beneficial here. Line 468-470 seems wrong now. Para
> line 476 also looks wrong.
Re-ordered the paragraphs and made them consistent.

There is an asymmetry between the reduced value computation.
For formatting, it is minWidth, on parsing it is two digits.
I would be inclined to change parsing to use minwidth but then it would not
match the SimpleDateFormat that is explicitly two digits.  In most
case it would have the same effect since "yy" converts to a fixed with of 2.
It affects test cases with the width == 1, not likely to used often.

>
> DTFB line 2685, 2715, 496, 543 Javadoc differ from what the code accepts.
Cleaned up to allow 1..10 characters.
>
>
> In TestReducedParser, the parseStrict tests under "Negative baseValue"
> do not all have negative bases, and perhaps they should. Same with
> parseLenient
Reordered the test cases and removed some duplicates.
>
> The TestReducedParser lenient tests has this
> {YEAR, 2, 4, 2000, "-10", 0, 3, 2090},
> which I think should be
> {YEAR, 2, 4, 2000, "-10", 0, 3, -10},
> as we should only map 01 to 99 to the base value (unless
> SimpleDateFormat does what your patch proposes)
SDF values are relative for exactly 2 parsed digits.
Updated ReducedPrinterParser to match.
>
> While this
> {"ddMMyy", "25062001",  LENIENT, 0, 8, 2001, 20, 2506},
> is clearly "wrong" as far as a human might read it, it is right as far
> as the rules defined are concerned. We could only get better if we
> used the knowledge of month range and/or have separate append methods
> for fixed width. That might be desirable, but is a lot more
> complicated.
Very consistent as is, but appendValueReduced could be special cased
to force the preceding fields to be fixed (as if it were a variable 
width field).
If the developer used the 2nd form of addValueReduced with two different 
widths
the problem would not appear.
>
>
> So, with all the great logic now in the appendValue and
> appendValueReduced, the only things that the new reduced method
> provide AFAICT are:
> - allow strict parsing to be more lenient
> - allow lenient parsing to control the maximum width
yes, that's the value add.

Roger

>
> I guess that is sufficient behaviour to justify the new method,
> although I suspect it will be little used.
> thanks
> Stephen
>
>
>
> On 29 April 2013 22:54, roger riggs <roger.riggs at oracle.com> wrote:
>> Hi,
>>
>> Webrev:
>>     http://cr.openjdk.java.net/~rriggs/webrev-lenient-parse-218/
>>
>> The logic for adjacent value mode has been consolidated a single method
>> that uses the current state of active parser and the new PrinterParser
>> to decide how to adjust the active and new PrinterParser.
>>
>> The first value printer parser becomes the active PrinterParser (and if
>> parsing
>> is set to lenient may consume a variable number of digits), subsequent
>> fixed width fields are forced to remain fixed width and their width
>> is contributed to the first value field to prevent unbounded digit
>> consumption.
>> If a variable width field is appended to the sequence, the initial active
>> value
>> field is modified to be fixed width so that the entire sequence is fixed and
>> the new variable width field becomes the active printer parser.
>> A ValueReducedPrinterParser is treated no differently.
>>
>> Please review the updated test cases for correctness in the lenient
>> and strict modes and the other changes.
>>
>> Thanks, Roger
>>