Looks good overall. Only comment is whether there should be a <p> tag after the </ul> and before "Outside the range". The resulting javadoc looks like it needs it, although I don't know what the OpenJDK house rule is for that case.
Stephen On 4 February 2015 at 21:17, Roger Riggs <roger.ri...@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.ri...@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.ri...@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.ri...@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.ander...@oracle.com> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> +1 >>>>>>>>> On Feb 3, 2015, at 3:45 PM, Roger Riggs <roger.ri...@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.ander...@oracle.com >>>>>>>>> >>>>>>>>> >>>>>>>>> >