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

Wed Feb 4 21:32:36 UTC 2015

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> 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/~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
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
> 

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