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