Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone()
I'm happy with this updated patch. Stephen On 24 March 2014 18:24, Xueming Shen wrote: > Hi Stephen, > > The webrev has been updated accordingly. > > http://cr.openjdk.java.net/~sherman/8033662/webrev/ > > Thanks! > -Sherman > > > On 03/24/2014 08:59 AM, Stephen Colebourne wrote: > > I don't think think email was delivered to me. (And I think another from > you also wasn't). The email is in the core-libs-dev archive though. > > I think the approach you changed it to is fine in principal although > whether the tidy up should be in jdk8u is a different matter. > > In your changes I think that toParsed() should be renamed to > toUnresolved(). And there still seem to be instance variables for > effectiveZone and effectiveChrono (which I thought you were trying to > remove). > > thanks > Stephen > > > > > > On 24 March 2014 15:00, Xueming Shen wrote: > >> >> Stephen, >> >> I commented on this one last week. I did not see the response, is it >> something worth considering? or >> you believe the original approach is the right way to go. >> >> Thanks! >> -Sherman >> >> Original Message Message-ID: >> <532b6f87.3000...@oracle.com> <532b6f87.3000...@oracle.com> Date: Thu, >> 20 Mar 2014 15:45:27 -0700 From: Xueming Shen >> User-Agent: >> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.17) Gecko/20110414 >> Thunderbird/3.1.10 MIME-Version: 1.0 To: core-libs-dev@openjdk.java.net >> Subject: >> Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone() >> References: >> >> >> In-Reply-To: >> >> Content-Type: >> text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: >> 7bit >> >> Hi Stephen, >> >> Given the fact that the parser context is no longer public, the parsed from >> the >> formatter is either unresolved or resolved, just wonder if we really need >> those >> "effective" chrono and zone fields in Parsed. It appears perfect for me to >> simply >> keep these info inside the parser context and return the unresolved and >> resolved >> based on the formatter's request, for example >> http://cr.openjdk.java.net/~sherman/8033662/webrev2/ >> >> -Sherman >> >> On 03/20/2014 11:24 AM, Stephen Colebourne wrote: >> > Hi there, >> > It would be great if I could get a review please. >> > >> > The patch is viewable in plain text at JIRA (for IP reasons): >> > https://bugs.openjdk.java.net/secure/attachment/19216/ParseWithZone.patch >> >> > >> > The same patch is viewable in a nice format at GitHub >> > https://gist.github.com/jodastephen/9505761 >> > >> > This really needs to make 8u20 IMO, so I need to get it into jdk9 first >> > thanks >> > Stephen >> > >> > >> > >> > On 12 March 2014 12:29, Stephen Colebourne >> > wrote: >> >> This is a request for review of this bug: >> >> https://bugs.openjdk.java.net/browse/JDK-8033662 >> >> and the duplicate: >> >> https://bugs.openjdk.java.net/browse/JDK-8033659 >> >> >> >> The javadoc of the method java.time.format.DateTimeFormatter::withZone >> >> says: >> >> "If no zone has been parsed, then this override zone will be included >> >> in the result of the parse where it can be used to build instants and >> >> date-times." >> >> However, the implementation doesn't obey this. >> >> >> >> This is a very unfortunate bug that makes some date-time parsing a lot >> >> harder. >> >> >> >> A second related bug in an egde case - not properly handling a >> >> ChronoZonedDateTime from TemporalField.resolve - is also tested for >> >> and fixed. >> >> >> >> >> >> Proposed patch: >> >> https://gist.github.com/jodastephen/9505761 >> >> The patch includes no spec changes. >> >> The patch includes new and refactored TCK tests. The new tests for >> >> withZone() and withChronology() are based on the spec. >> >> >> >> I need a reviewer and a committer please. >> >> thanks >> >> >> >> >> > >
Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone()
Hi Stephen, The webrev has been updated accordingly. http://cr.openjdk.java.net/~sherman/8033662/webrev/ Thanks! -Sherman On 03/24/2014 08:59 AM, Stephen Colebourne wrote: I don't think think email was delivered to me. (And I think another from you also wasn't). The email is in the core-libs-dev archive though. I think the approach you changed it to is fine in principal although whether the tidy up should be in jdk8u is a different matter. In your changes I think that toParsed() should be renamed to toUnresolved(). And there still seem to be instance variables for effectiveZone and effectiveChrono (which I thought you were trying to remove). thanks Stephen On 24 March 2014 15:00, Xueming Shen mailto:xueming.s...@oracle.com>> wrote: Stephen, I commented on this one last week. I did not see the response, is it something worth considering? or you believe the original approach is the right way to go. Thanks! -Sherman Original Message Message-ID: <532b6f87.3000...@oracle.com> <mailto:532b6f87.3000...@oracle.com> Date: Thu, 20 Mar 2014 15:45:27 -0700 From: Xueming Shen <mailto:xueming.s...@oracle.com> User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.17) Gecko/20110414 Thunderbird/3.1.10 MIME-Version: 1.0 To: core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> Subject: Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone() References: <mailto:CACzrW9BTWhehV+oC-aEQAEkLtTA4XO6z-F5=jevyv-d35+y...@mail.gmail.com> <mailto:CACzrW9BcmmesdKQBrtgrAc=12hn3uowjvwzxskmfcksajea...@mail.gmail.com> In-Reply-To: <mailto:CACzrW9BcmmesdKQBrtgrAc=12hn3uowjvwzxskmfcksajea...@mail.gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi Stephen, Given the fact that the parser context is no longer public, the parsed from the formatter is either unresolved or resolved, just wonder if we really need those "effective" chrono and zone fields in Parsed. It appears perfect for me to simply keep these info inside the parser context and return the unresolved and resolved based on the formatter's request, for example http://cr.openjdk.java.net/~sherman/8033662/webrev2/ <http://cr.openjdk.java.net/%7Esherman/8033662/webrev2/> -Sherman On 03/20/2014 11:24 AM, Stephen Colebourne wrote: > Hi there, > It would be great if I could get a review please. > > The patch is viewable in plain text at JIRA (for IP reasons): > https://bugs.openjdk.java.net/secure/attachment/19216/ParseWithZone.patch > > The same patch is viewable in a nice format at GitHub > https://gist.github.com/jodastephen/9505761 > > This really needs to make 8u20 IMO, so I need to get it into jdk9 first > thanks > Stephen > > > > On 12 March 2014 12:29, Stephen Colebourne <mailto:scolebou...@joda.org> wrote: >> This is a request for review of this bug: >> https://bugs.openjdk.java.net/browse/JDK-8033662 >> and the duplicate: >> https://bugs.openjdk.java.net/browse/JDK-8033659 >> >> The javadoc of the method java.time.format.DateTimeFormatter::withZone says: >> "If no zone has been parsed, then this override zone will be included >> in the result of the parse where it can be used to build instants and >> date-times." >> However, the implementation doesn't obey this. >> >> This is a very unfortunate bug that makes some date-time parsing a lot harder. >> >> A second related bug in an egde case - not properly handling a >> ChronoZonedDateTime from TemporalField.resolve - is also tested for >> and fixed. >> >> >> Proposed patch: >> https://gist.github.com/jodastephen/9505761 >> The patch includes no spec changes. >> The patch includes new and refactored TCK tests. The new tests for >> withZone() and withChronology() are based on the spec. >> >> I need a reviewer and a committer please. >> thanks
Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone()
Hi Stephen, Given the fact that the parser context is no longer public, the parsed from the formatter is either unresolved or resolved, just wonder if we really need those "effective" chrono and zone fields in Parsed. It appears perfect for me to simply keep these info inside the parser context and return the unresolved and resolved based on the formatter's request, for example http://cr.openjdk.java.net/~sherman/8033662/webrev2/ -Sherman On 03/20/2014 11:24 AM, Stephen Colebourne wrote: Hi there, It would be great if I could get a review please. The patch is viewable in plain text at JIRA (for IP reasons): https://bugs.openjdk.java.net/secure/attachment/19216/ParseWithZone.patch The same patch is viewable in a nice format at GitHub https://gist.github.com/jodastephen/9505761 This really needs to make 8u20 IMO, so I need to get it into jdk9 first thanks Stephen On 12 March 2014 12:29, Stephen Colebourne wrote: This is a request for review of this bug: https://bugs.openjdk.java.net/browse/JDK-8033662 and the duplicate: https://bugs.openjdk.java.net/browse/JDK-8033659 The javadoc of the method java.time.format.DateTimeFormatter::withZone says: "If no zone has been parsed, then this override zone will be included in the result of the parse where it can be used to build instants and date-times." However, the implementation doesn't obey this. This is a very unfortunate bug that makes some date-time parsing a lot harder. A second related bug in an egde case - not properly handling a ChronoZonedDateTime from TemporalField.resolve - is also tested for and fixed. Proposed patch: https://gist.github.com/jodastephen/9505761 The patch includes no spec changes. The patch includes new and refactored TCK tests. The new tests for withZone() and withChronology() are based on the spec. I need a reviewer and a committer please. thanks
Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone()
The changes look ok to me Stephen. I defer to others for the handling of the test/TCK changes. -Chris. > On 20 Mar 2014, at 18:24, Stephen Colebourne wrote: > > Hi there, > It would be great if I could get a review please. > > The patch is viewable in plain text at JIRA (for IP reasons): > https://bugs.openjdk.java.net/secure/attachment/19216/ParseWithZone.patch > > The same patch is viewable in a nice format at GitHub > https://gist.github.com/jodastephen/9505761 > > This really needs to make 8u20 IMO, so I need to get it into jdk9 first > thanks > Stephen > > > >> On 12 March 2014 12:29, Stephen Colebourne wrote: >> This is a request for review of this bug: >> https://bugs.openjdk.java.net/browse/JDK-8033662 >> and the duplicate: >> https://bugs.openjdk.java.net/browse/JDK-8033659 >> >> The javadoc of the method java.time.format.DateTimeFormatter::withZone says: >> "If no zone has been parsed, then this override zone will be included >> in the result of the parse where it can be used to build instants and >> date-times." >> However, the implementation doesn't obey this. >> >> This is a very unfortunate bug that makes some date-time parsing a lot >> harder. >> >> A second related bug in an egde case - not properly handling a >> ChronoZonedDateTime from TemporalField.resolve - is also tested for >> and fixed. >> >> >> Proposed patch: >> https://gist.github.com/jodastephen/9505761 >> The patch includes no spec changes. >> The patch includes new and refactored TCK tests. The new tests for >> withZone() and withChronology() are based on the spec. >> >> I need a reviewer and a committer please. >> thanks
Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone()
Hi there, It would be great if I could get a review please. The patch is viewable in plain text at JIRA (for IP reasons): https://bugs.openjdk.java.net/secure/attachment/19216/ParseWithZone.patch The same patch is viewable in a nice format at GitHub https://gist.github.com/jodastephen/9505761 This really needs to make 8u20 IMO, so I need to get it into jdk9 first thanks Stephen On 12 March 2014 12:29, Stephen Colebourne wrote: > This is a request for review of this bug: > https://bugs.openjdk.java.net/browse/JDK-8033662 > and the duplicate: > https://bugs.openjdk.java.net/browse/JDK-8033659 > > The javadoc of the method java.time.format.DateTimeFormatter::withZone says: > "If no zone has been parsed, then this override zone will be included > in the result of the parse where it can be used to build instants and > date-times." > However, the implementation doesn't obey this. > > This is a very unfortunate bug that makes some date-time parsing a lot harder. > > A second related bug in an egde case - not properly handling a > ChronoZonedDateTime from TemporalField.resolve - is also tested for > and fixed. > > > Proposed patch: > https://gist.github.com/jodastephen/9505761 > The patch includes no spec changes. > The patch includes new and refactored TCK tests. The new tests for > withZone() and withChronology() are based on the spec. > > I need a reviewer and a committer please. > thanks
Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone()
ping On 12 March 2014 12:29, Stephen Colebourne wrote: > This is a request for review of this bug: > https://bugs.openjdk.java.net/browse/JDK-8033662 > and the duplicate: > https://bugs.openjdk.java.net/browse/JDK-8033659 > > The javadoc of the method java.time.format.DateTimeFormatter::withZone says: > "If no zone has been parsed, then this override zone will be included > in the result of the parse where it can be used to build instants and > date-times." > However, the implementation doesn't obey this. > > This is a very unfortunate bug that makes some date-time parsing a lot harder. > > A second related bug in an egde case - not properly handling a > ChronoZonedDateTime from TemporalField.resolve - is also tested for > and fixed. > > > Proposed patch: > https://gist.github.com/jodastephen/9505761 > The patch includes no spec changes. > The patch includes new and refactored TCK tests. The new tests for > withZone() and withChronology() are based on the spec. > > I need a reviewer and a committer please. > thanks
