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

2015-02-04 Thread Stephen Colebourne
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

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

2015-02-04 Thread Roger Riggs
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

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

2015-02-04 Thread Lance Andersen
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.ri...@oracle.com wrote: Hi

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

2015-02-04 Thread Roger Riggs
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

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

2015-02-04 Thread Roger Riggs
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

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

2015-02-04 Thread Lance Andersen
Hi Roger, I think I know what this is trying to say, but if possible, could you provide the revised javadoc so it is easier to see the full context? Best Lance On Feb 4, 2015, at 2:08 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi, Clarifying that Chronology.isLeapYear is specified only

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

2015-02-04 Thread Stephen Colebourne
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 * lia leap-year must imply a year-length longer than a non

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

2015-02-04 Thread Roger Riggs
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

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

2015-02-04 Thread Stephen Colebourne
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

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

2015-02-04 Thread Roger Riggs
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

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

2015-02-04 Thread Stephen Colebourne
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. Right now, this patch makes a subclass implementation incompatible with the superclass spec. That cannot happen. Thus there are only

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

2015-02-03 Thread Roger Riggs
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

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

2015-02-03 Thread Lance Andersen
+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:

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

2015-02-03 Thread Stephen Colebourne
-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