I'm happy with this updated patch. Stephen
On 24 March 2014 18:24, Xueming Shen <xueming.s...@oracle.com> 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 <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> <532b6f87.3000...@oracle.com> Date: Thu, >> 20 Mar 2014 15:45:27 -0700 From: Xueming Shen >> <xueming.s...@oracle.com><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 >> Subject: >> Re: RFR: JDK-8033662 DateTimeFormatter parsing ignores withZone() >> References: >> <CACzrW9BTWhehV+oC-aEQAEkLtTA4XO6z-F5=jevyv-d35+y...@mail.gmail.com><CACzrW9BTWhehV+oC-aEQAEkLtTA4XO6z-F5=jevyv-d35+y...@mail.gmail.com> >> <CACzrW9BcmmesdKQBrtgrAc=12hn3uowjvwzxskmfcksajea...@mail.gmail.com><CACzrW9BcmmesdKQBrtgrAc=12hn3uowjvwzxskmfcksajea...@mail.gmail.com> >> In-Reply-To: >> <CACzrW9BcmmesdKQBrtgrAc=12hn3uowjvwzxskmfcksajea...@mail.gmail.com><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/ >> >> -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<scolebou...@joda.org> >> > <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 >> >> >> >> >> > >
