Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Wed, 11 May 2022 08:32:48 GMT, Uwe Schindler wrote: >> Can the test cover `UT` prefix as well? (This is another valid prefix in >> `ZoneId`) >> >> If this PR isn't meant to work with UTC prefix, can a test be added that >> proves it does *not* work. >> >> ie. all these are valid in `ZoneId` - "Z", "UTC", "GMT", "UT", "UTC+01:00", >> "GMT+01:00", "UT+01:00" - and all should have some form of associated test. > > I tried it out: If you create a ZoneId with prefixes "UT" and "UTC", they > fail to convert to TimeZone. Same happens if you use this as String name in > `TimeZone#getTimeZone(String)`. This is another bug / missing feature! It > does not work with or without this PR. In short, we can only test what works. The test was mainly added to allow roundtrips of `ZoneOffset` instances. - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Wed, 11 May 2022 08:10:56 GMT, Stephen Colebourne wrote: >> Added them except "UTC+...", as it is not recognizable as a Custom ID. > > Can the test cover `UT` prefix as well? (This is another valid prefix in > `ZoneId`) > > If this PR isn't meant to work with UTC prefix, can a test be added that > proves it does *not* work. > > ie. all these are valid in `ZoneId` - "Z", "UTC", "GMT", "UT", "UTC+01:00", > "GMT+01:00", "UT+01:00" - and all should have some form of associated test. I tried it out: If you create a ZoneId with prefixes "UT" and "UTC", they fail to convert to TimeZone. Same happens if you use this as String name in `TimeZone#getTimeZone(String)`. This is another bug / missing feature! It does not work with or without this PR. - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Tue, 10 May 2022 17:43:07 GMT, Naoto Sato wrote:
>> test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43:
>>
>>> 41: private Object[][] testZoneOffsets() {
>>> 42: return new Object[][] {
>>> 43: {ZoneId.of("Z"), 0},
>>
>> I know, `ZoneId.of()` should parse this as a `ZoneOffset` and return a
>> `ZoneOffset` instance, but maybe add also the other string variants with
>> prefix (`ZoneId.of("UTC+00:00:01")` or `ZoneId.of("GMT+00:00:01")` as data
>> items. Maybe also use `ZoneOffset.of()` for the plain zones to be explicit.
>
> Added them except "UTC+...", as it is not recognizable as a Custom ID.
Can the test cover `UT` prefix as well? (This is another valid prefix in
`ZoneId`)
If this PR isn't meant to work with UTC prefix, can a test be added that proves
it does *not* work.
ie. all these are valid in `ZoneId` - "Z", "UTC", "GMT", "UT", "UTC+01:00",
"GMT+01:00", "UT+01:00" - and all should have some form of associated test.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Tue, 10 May 2022 17:43:24 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/util/TimeZone.java line 543:
>>
>>> 541: return new ZoneInfo(totalSecs == 0 ? "UTC" : GMT_ID +
>>> tzid, totalSecs);
>>> 542: } else {
>>> 543: return getTimeZone(tzid, true);
>>
>> Before the change in this PR, we used to prefix `GMT` to (non-custom
>> timezone ids) if the timezone id returned by `ZoneId#getId()` started with
>> the `+` or `-` sign, before calling `getTimeZone(modifiedTzid, true)`.
>> With this change, for `ZoneId`s that aren't `ZoneOffset` instance, we now
>> call `getTimeZone(originalTzid, true)`, without first checking/prefixing the
>> id with `GMT`. Is that an intentional change and would that potentially
>> cause `getTimeZone(String, boolean)` to return a different result?
>
> Yes, it is intentional. The `Time-zone IDs` section in the `ZoneId` class
> description is clear that zone id starting with "+/-" is a `ZoneOffset`
> instance. Other ZoneIds should have offsets with prefix or region-based ids.
I was stumbling on this, too, but it is only ZoneOffset isntances that have
those strange names without GMT/UTC prefix. Default ZoneId instances created
from text strings always have a prefix. If you call ZoneId.of() with an offset
only and no prefix it returns a ZoneOffset instance (see the test).
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Mon, 9 May 2022 22:29:50 GMT, Uwe Schindler wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed offsets in milliseconds, added test variations, refined Custom ID
>> definitions
>
> src/java.base/share/classes/java/util/TimeZone.java line 539:
>
>> 537: public static TimeZone getTimeZone(ZoneId zoneId) {
>> 538: String tzid = zoneId.getId(); // throws an NPE if null
>> 539: if (zoneId instanceof ZoneOffset zo) {
>
> I like this because it is much faster than the conversion to ZoneId and
> parsing it back! It is similar to use of SimpleTimeZone, but this is better
> as the returned timezone is unmodifiable, correct?
Yes, and it aligns with the other call site (line 588).
> test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43:
>
>> 41: private Object[][] testZoneOffsets() {
>> 42: return new Object[][] {
>> 43: {ZoneId.of("Z"), 0},
>
> I know, `ZoneId.of()` should parse this as a `ZoneOffset` and return a
> `ZoneOffset` instance, but maybe add also the other string variants with
> prefix (`ZoneId.of("UTC+00:00:01")` or `ZoneId.of("GMT+00:00:01")` as data
> items. Maybe also use `ZoneOffset.of()` for the plain zones to be explicit.
Added them except "UTC+...", as it is not recognizable as a Custom ID.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Tue, 10 May 2022 13:12:10 GMT, Jaikiran Pai wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed offsets in milliseconds, added test variations, refined Custom ID
>> definitions
>
> src/java.base/share/classes/java/util/TimeZone.java line 109:
>
>> 107: *
>> 108: * NormalizedCustomID:
>> 109: * {@code GMT} Sign TwoDigitHours {@code :}
>> Minutes [Seconds]
>
> Hello Naoto,
>
> Should this instead be: `... Minutes [{@code :} Seconds]` -
> i.e. should it have the `:` literal if seconds are present in the custom
> timezone id?
The colon is included in `Seconds` part below. I changed the part name to
`ColonSeconds` to make it clearer.
> src/java.base/share/classes/java/util/TimeZone.java line 543:
>
>> 541: return new ZoneInfo(totalSecs == 0 ? "UTC" : GMT_ID + tzid,
>> totalSecs);
>> 542: } else {
>> 543: return getTimeZone(tzid, true);
>
> Before the change in this PR, we used to prefix `GMT` to (non-custom timezone
> ids) if the timezone id returned by `ZoneId#getId()` started with the `+` or
> `-` sign, before calling `getTimeZone(modifiedTzid, true)`.
> With this change, for `ZoneId`s that aren't `ZoneOffset` instance, we now
> call `getTimeZone(originalTzid, true)`, without first checking/prefixing the
> id with `GMT`. Is that an intentional change and would that potentially cause
> `getTimeZone(String, boolean)` to return a different result?
Yes, it is intentional. The `Time-zone IDs` section in the `ZoneId` class
description is clear that zone id starting with "+/-" is a `ZoneOffset`
instance. Other ZoneIds should have offsets with prefix or region-based ids.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed offsets in milliseconds, added test variations, refined Custom ID definitions - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/843e86be..81a806e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8606=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8606=00-01 Stats: 13 lines in 2 files changed: 3 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
