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

Roger Riggs Tue, 15 Mar 2016 12:22:26 -0700

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 theprecision constant and "Zone" for the ZONE constant, as they describethe 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 innerclasses andadded toString methods that returned the unqualified type name, usefulfor 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 problemand is a cornercase.

There is no change in specified behavior and the new language isinformative

using 'typically' to clarify the correct use of the formatters.

I augmented the implementation tests to exercise the case and checkthe 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
    <mailto: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/
        <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.ri...@oracle.com <mailto: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/
                <http://cr.openjdk.java.net/%7Erriggs/webrev-format-8085887/>

                Thanks, Roger

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

Reply via email to