RFR 9: 8085887 : java.time.format.FormatStyle.LONG or FULL causes unchecked exception

[Stephen Colebourne]

Great to see the better error messages. It may be better long term to
add toString() to the TemporalQuery implementations (lambdas are
convenient but optional). That said, what is in the webrev is what is
needed here.

The error message in the inner class probably ought to be more clever,
although what is added is a lot better than before. Ultimately, it
should capture the temporal being formatted.

return temporal.toString() +
  (overrideChrono != null ? " with chronology + overrideChrono : "") +
  (overrideZone != null ? " with zone + overrideZone : "");

The DateTimeFormatterBuilder spec change for parsing may not make
sense. When parsing, the zone is driven by the localized pattern, so
getZone() only comes into play if trying to query ZDT from a pattern
where the zone was NOT parsed. As such, I'm not sure the spec needs
clarifying in the parse section.

thanks
Stephen


On 14 March 2016 at 20:17, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> Hi Stephen,
>
> Thanks for the comments and suggestions.
>
> I have no issue cleaning it all up in one change.
>
> Updated webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-format-8085887/
>
> On 3/12/2016 4:47 AM, Stephen Colebourne wrote:
>>
>> I think the new text needs should be more specific:
>>
>> The {@code FULL} and {@code LONG} styles typically require a time-zone.
>> When formatting using these styles, a {@code ZoneId} must be available,
>> either by using {@code ZonedDateTime} or {@link
>> DateTimeFormatter#withZone}.
>
> ok
>>
>>
>> (testing shows that the other two styles appear to not use the time-zone.)
>>
>> While it would be nice to support these styles when no time-zone is
>> available, that seems like an unrealistic goal.
>>
>>
>> Beyond this, the error message is terrible, eg:
>> Unable to extract value: class java.time.format.DateTimePrintContext$1
>> Unable to extract value: class java.time.LocalDateTime
>> Unable to extract value: class java.time.OffsetDateTime
>>
>> This requires two separate changes.
>> 1) Add toString() to the inner class at line 185 in DateTimePrintContext
>
> Delegates to the temporal.
>>
>>
>> 2) At line 282 in DateTimePrintContext, the exception message needs to
>> record what is being queried - ZoneId, ZoneOffset, Chronology etc.
>> Something like:
>> if (query == TemporalQueries.zoneId() {
>>   throw new DateTimeException("Unable to extract ZoneId from temporal:
>> " + temporal.getClass());
>> } else if (query == TemporalQueries.zoneOffset() {
>>   throw new DateTimeException("Unable to extract ZoneOffset from
>> temporal: " + temporal.getClass());
>> } else ...
>
> An  more general alternative would be to add toString methods to each of the
> TemporalQuery instances
> but that would mean giving up the use of lambda for their implementations.
>
> Thanks, Roger
>
>
>>
>> I'm happy for the error message problem to be fixed in a different
>> issue if desired.
>>
>> Stephen
>>
>>
>>
>> On 11 March 2016 at 20:19, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>>>
>>> Please review java.time javadoc improvements to highlight a common
>>> misunderstanding
>>> about formatting elements that require a timezone in addition to the
>>> time.
>>> When using locale specific formatting, it may work if the locale
>>> formatting
>>> does not
>>> require a timezone or fail if the locale formatting requires a timezone
>>> and
>>> a timezone is not provided.
>>>
>>> ZoneDateTime or OffsetDateTime types should be used when formatting with
>>> locale
>>> dependent formatters.
>>>
>>> 8085887 : java.time.format.FormatStyle.LONG or FULL causes unchecked
>>> exception
>>>
>>> Webrev:
>>>    http://cr.openjdk.java.net/~rriggs/webrev-format-8085887/
>>>
>>> Thanks, Roger
>>>
>