RFR 9: 8067800: Unexpected DateTimeException in the java.time.chrono.HijrahChronology.isLeapYear

Wed Feb 4 21:47:49 UTC 2015

Thanks Lance,  I updated to use assertFalse.

On 2/4/2015 4:32 PM, Lance Andersen wrote:
> Hi Roger,
>
> I think it looks fine.
>
> If you wanted to and not necessary, you could use assertFalse vs 
> assertEquals in your test (more of a style choice )
>
>
> Thank you for the javadoc as it is clearer that way
>
> Best
> Lance
> On Feb 4, 2015, at 4:17 PM, Roger Riggs <Roger.Riggs at oracle.com 
> <mailto:Roger.Riggs at oracle.com>> wrote:
>
>> Hi Stephen,
>>
>> That covers the cases better.
>>
>> The updated javadoc is:
>> http://cr.openjdk.java.net/~rriggs/xx/java/time/chrono/Chronology.html <http://cr.openjdk.java.net/%7Erriggs/xx/java/time/chrono/Chronology.html>
>> http://cr.openjdk.java.net/~rriggs/xx/java/time/chrono/HijrahChronology.html
>>
>> Webrev:
>>   http://cr.openjdk.java.net/~rriggs/webrev-leap-year-8067800/
>>
>> Roger
>>
>>
>>
>> On 2/4/2015 3:42 PM, Stephen Colebourne wrote:
>>> I think this might be clearer:
>>>
>>> * Checks if the specified year is a leap year.
>>> * <p>
>>> * A leap-year is a year of a longer length than normal.
>>> * The exact meaning is determined by the chronology according to the
>>> following constraints.
>>> * <ul>
>>> * <li>a leap-year must imply a year-length longer than a non leap-year.
>>> * <li>a chronology that does not support the concept of a year must
>>> return false.
>>> * <li>the correct result must be returned for all years within the
>>> valid range of years for the chronology
>>> * </ul>
>>> * Outside the range of valid years an implementation is free to return
>>> either a best guess or false.
>>> * An implementation must not throw an exception, even if the year is
>>> outside the range of valid years..
>>>
>>> Stephen
>>>
>>>
>>>
>>> On 4 February 2015 at 19:08, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>>>> Hi,
>>>>
>>>> Clarifying that Chronology.isLeapYear is specified only within the 
>>>> range of
>>>> the chronology
>>>> makes it possible to maintain the invariants with the calendar 
>>>> computations
>>>> and methods.
>>>> Best effort isn't testable except in an implementation specific way and
>>>> can't be relied on.
>>>>
>>>> The other two constraints are testable and use 'must' in their 
>>>> definitions.
>>>> The new phrasing should clearly be either an exception or reinforce the
>>>> value is
>>>> specified only within the range of the chronology.
>>>>
>>>> How about:
>>>>
>>>> <li>except if the year is out of range the chronology should return 
>>>> a best
>>>> effort guess, or false if there is no suitable guess
>>>>
>>>> Roger
>>>>
>>>>
>>>>
>>>> On 2/4/2015 11:18 AM, Stephen Colebourne wrote:
>>>>> The spec is pretty clear:
>>>>> "the proleptic-year to check, not validated for range"
>>>>>
>>>>> Adding an exception to the spec would cause requests to add exceptions
>>>>> to all other chronologies. Doing so, would be very negative to
>>>>> performance (it would require having a non-public copy of the logic
>>>>> elsewhere to avoid the range checking cost.
>>>>>
>>>>> Adding an exception to Hijrah only means that the implementation is
>>>>> doing something not permitted by the spec.
>>>>>
>>>>> The only sane choice here is to return false from Hijrah when out of
>>>>> range. As intended and as specced.
>>>>>
>>>>> Note that I don't believe that returning false will cause hardship in
>>>>> any actual user code, because other methods will constrain the year to
>>>>> be valid.
>>>>>
>>>>> If necessary, the following spec clarification could be added to the
>>>>> bullet points on Chronology:
>>>>>
>>>>> <li>if the year is out of range the chronology should return a best
>>>>> effort guess, or false if there is no suitable guess
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>>> On 4 February 2015 at 16:05, Roger Riggs <Roger.Riggs at oracle.com> 
>>>>> wrote:
>>>>>> Hi Stephen,
>>>>>>
>>>>>> On 2/4/15 10:43 AM, Stephen Colebourne wrote:
>>>>>>> The java.time code generally takes a lenient approach to methods 
>>>>>>> that
>>>>>>> return a boolean. For example, they tend to accept null inputs 
>>>>>>> without
>>>>>>> throwing an exception.
>>>>>> Seems like an odd design provision leading to some behavior that
>>>>>> would be inconsistent with non-boolean methods.
>>>>>>>
>>>>>>> Right now, this patch makes a subclass implementation incompatible
>>>>>>> with the superclass spec. That cannot happen. Thus there are 
>>>>>>> only two
>>>>>>> options:
>>>>>>>
>>>>>>> - change the superclass spec
>>>>>>> - return false from the subclass
>>>>>>>
>>>>>>> I favour the latter as being in line with the rest of the 
>>>>>>> package and
>>>>>>> less disruptive to existing users (would a CCC even pass?)
>>>>>> For a 'young' API with limited uptake, issues can be fixed early 
>>>>>> and in a
>>>>>> major
>>>>>> release there is more flexibility to clarify the specification.
>>>>>>
>>>>>> DTE is a runtime exception and can occur in a majority of time
>>>>>> computations.
>>>>>> In the case of HijrahChronolog.isLeapYear, the implementation 
>>>>>> currently
>>>>>> throws a different RuntimeException and this would be a correction
>>>>>> to the conventional exception.
>>>>>>
>>>>>> Roger
>>>>>>
>>>>>>
>>>>>>> Stephen
>>>>>>>
>>>>>>>
>>>>>>> On 4 February 2015 at 15:10, Roger Riggs 
>>>>>>> <Roger.Riggs at oracle.com> wrote:
>>>>>>>> Hi Stephen,
>>>>>>>>
>>>>>>>> I also indicated in the Jira comments that it is misleading and
>>>>>>>> incorrect
>>>>>>>> to
>>>>>>>> return
>>>>>>>> false when it is not known that a year is or is not a leap 
>>>>>>>> year. All of
>>>>>>>> the
>>>>>>>> other
>>>>>>>> HijrahChronology computations throw DTE for invalid dates and 
>>>>>>>> the same
>>>>>>>> may
>>>>>>>> be
>>>>>>>> true for other Chronologies.
>>>>>>>>
>>>>>>>> The assertion in Chronology.isLeapYear about not checking the 
>>>>>>>> range is
>>>>>>>> too
>>>>>>>> broad
>>>>>>>> and should be qualified by the Chronology.
>>>>>>>>
>>>>>>>> Perhaps the proposed change should include a caveat in that method.
>>>>>>>>
>>>>>>>> Roger
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/3/15 8:05 PM, Stephen Colebourne wrote:
>>>>>>>>> -1
>>>>>>>>>
>>>>>>>>> As I indicated on JIRA, I don't believe that this change meets the
>>>>>>>>> spec or intent of the definition on Chronology. That is 
>>>>>>>>> specified to
>>>>>>>>> not throw any exceptions and to handle all years, valid or not.
>>>>>>>>>
>>>>>>>>> I don't foresee any significant issue where a year is not 
>>>>>>>>> validated by
>>>>>>>>> this method. Years out of range should simply return false, again
>>>>>>>>> something that is within the spirit of the spec "a chronology that
>>>>>>>>> does not support the concept of a year must return false."
>>>>>>>>>
>>>>>>>>> Stephen
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3 February 2015 at 20:56, Lance Andersen
>>>>>>>>> <lance.andersen at oracle.com>
>>>>>>>>> wrote:
>>>>>>>>>> +1
>>>>>>>>>> On Feb 3, 2015, at 3:45 PM, Roger Riggs <Roger.Riggs at oracle.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Please review this specification clarification of the range of
>>>>>>>>>>> Hijrah
>>>>>>>>>>> calendar variants.
>>>>>>>>>>> The issue was exposed by a bug in the 
>>>>>>>>>>> HijrahChronology.isLeapYear
>>>>>>>>>>> method.
>>>>>>>>>>>
>>>>>>>>>>> Webrev:
>>>>>>>>>>>      http://cr.openjdk.java.net/~rriggs/webrev-leap-year-8067800/
>>>>>>>>>>>
>>>>>>>>>>> Issue:
>>>>>>>>>>>      https://bugs.openjdk.java.net/browse/JDK-8067800
>>>>>>>>>>>
>>>>>>>>>>> A CCC may be needed.
>>>>>>>>>>>
>>>>>>>>>>> Thanks, Roger
>>>>>>>>>>>
>>>>>>>>>> Lance Andersen| Principal Member of Technical Staff | 
>>>>>>>>>> +1.781.442.2037
>>>>>>>>>> Oracle Java Engineering
>>>>>>>>>> 1 Network Drive
>>>>>>>>>> Burlington, MA 01803
>>>>>>>>>> Lance.Andersen at oracle.com
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>
>
>