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

[Xueming Shen]

looks good.

On 3/15/16 12:19 PM, Roger Riggs wrote:
> Hi Stephen,
>
> Updated as recommended.
>
> Thanks,  Roger
>
>
> On 3/15/2016 2:52 PM, Stephen Colebourne wrote:
>>
>> My only comment is that I would prefer to see "Precision" for the 
>> precision constant and "Zone" for the ZONE constant, as they describe 
>> the query better. No need for another webrev.
>>
>> Otherwise a good improvement. Thanks.
>> Stephen
>>
>> Hi Stephen,
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-format-8085887/ 
>> <http://cr.openjdk.java.net/%7Erriggs/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
>>     <mailto: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/
>> <http://cr.openjdk.java.net/%7Erriggs/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 <mailto: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/
>> <http://cr.openjdk.java.net/%7Erriggs/webrev-format-8085887/>
>>
>>                 Thanks, Roger
>>
>>
>