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

[Roger Riggs]

Hi Stephen,

Updated webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-format-8085887/


On 3/14/2016 7:45 PM, Stephen Colebourne wrote:
> 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.
Agreed, converted the TemporalQuery implementations to anonymous inner 
classes and
added toString methods that returned the unqualified type name, useful 
for messages and debugging.
>
> 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 : "");
Yes, an improvement
>
> 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.
Removed the section on parsing, it has not been reported as a problem 
and is a cornercase.

There is no change in specified behavior and the new language is informative
using 'typically' to clarify the correct use of the formatters.

I augmented the implementation tests to exercise the case and check the 
exception message
for the useful types and values.

Thanks, Roger

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