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

[Stephen Colebourne]

Looks good overall.
Only comment is whether there should be a  tag after the  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  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.
>> * 
>> * 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.
>> * 
>> * a leap-year must imply a year-length longer than a non leap-year.
>> * a chronology that does not support the concept of a year must
>> return false.
>> * the correct result must be returned for all years within the
>> valid range of years for the chronology
>> * 
>> * 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  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:
>>>
>>> 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:

 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  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 
>> 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. 

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

[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

Best
Lance
On Feb 4, 2015, at 4:17 PM, Roger Riggs > 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.
* 
* 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.
* 
* a leap-year must imply a year-length longer than a non leap-year.
* a chronology that does not support the concept of a year must
return false.
* the correct result must be returned for all years within the
valid range of years for the chronology
* 
* 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  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:

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:

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

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

[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  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.
>> * 
>> * 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.
>> * 
>> * a leap-year must imply a year-length longer than a non leap-year.
>> * a chronology that does not support the concept of a year must
>> return false.
>> * the correct result must be returned for all years within the
>> valid range of years for the chronology
>> * 
>> * 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  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:
>>> 
>>> 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:
 
 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  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  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 

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

[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



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.
* 
* 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.
* 
* a leap-year must imply a year-length longer than a non leap-year.
* a chronology that does not support the concept of a year must
return false.
* the correct result must be returned for all years within the
valid range of years for the chronology
* 
* 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  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:

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:

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

wrote:

+1
On Feb 3, 2015, at 3:45 PM,

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

[Stephen Colebourne]

I think this might be clearer:

* Checks if the specified year is a leap year.
* 
* 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.
* 
* a leap-year must imply a year-length longer than a non leap-year.
* a chronology that does not support the concept of a year must
return false.
* the correct result must be returned for all years within the
valid range of years for the chronology
* 
* 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  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:
>
> 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:
>>
>> 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  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  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 th

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

[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  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:
> 
> 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:
>> 
>> 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  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  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 
>> wrote:
>>> +1
>>> On Feb 3, 2015, at 3:45 PM, Roger Riggs  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-

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

[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 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:

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:

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  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  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 
wrote:

+1
On Feb 3, 2015, at 3:45 PM, Roger Riggs  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

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

[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 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:

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  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  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 
 wrote:
>
> +1
> On Feb 3, 2015, at 3:45 PM, Roger Riggs  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
>
>
>
>

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

[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 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  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 
wrote:

+1
On Feb 3, 2015, at 3:45 PM, Roger Riggs  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

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

[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 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?)

Stephen


On 4 February 2015 at 15:10, Roger Riggs  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 
>> wrote:
>>>
>>> +1
>>> On Feb 3, 2015, at 3:45 PM, Roger Riggs  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
>>>
>>>
>>>
>

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

[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 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  wrote:

+1
On Feb 3, 2015, at 3:45 PM, Roger Riggs  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

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

[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 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  wrote:
> +1
> On Feb 3, 2015, at 3:45 PM, Roger Riggs  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
>
>
>

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

[Lance Andersen]

+1
On Feb 3, 2015, at 3:45 PM, Roger Riggs  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