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

[Roger Riggs]

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