-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8339637: (tz) Update Timezone Data to 2024b #21265
Conversation
👋 Welcome back johnyjose30! A progress list of the required criteria for merging this PR into |
@johnyjose30 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 310 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@justin-curtis-lu, @naotoj) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@johnyjose30 Reviewer |
@johnyjose30 The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
* later, where 'EST', 'MST' and 'HST' map to IDs which do not include daylight | ||
* savings. | ||
* savings since 1970. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Johny, the future flexibility clause seems to be missing in the spec update of ZoneId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes in c323844.
Can we also add the following to the ZoneId specification,
* savings since 1970. | |
* savings since 1970. This mapping may change in update releases in support of new versions of TZDB. |
} | ||
aliases.put("HST", "Pacific/Honolulu"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct to me, since "HST" to "Pacific/Honolulu" link is same for USE_OLDMAPPING
as well as 2024b update.
@@ -186,15 +186,12 @@ public abstract sealed class ZoneId implements Serializable permits ZoneOffset, | |||
* This map allows the IDs to continue to be used via the | |||
* {@link #of(String, Map)} factory method. | |||
* <p> | |||
* This map contains a mapping of the IDs that is in line with TZDB 2005r and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this file needs a copyright year bump.
@@ -35,7 +35,6 @@ | |||
public class Bug4848242 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test needs a copyright year bump. (Applies to any other tests as well).
We also might want to update the summary, since it should no longer display MET and MEST, but rather CET and CEST.
|
||
if (l1.equals(l2)) { | ||
System.out.println("no diff"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? IIUC diff
is only called if the array contents are not equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since diff
is a generic debug function, equality check needed to be in the function itself. Anyway, I wrote this code for purely debug purpose, so can be removed altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides small comment, looks correct to me, thanks for the changes.
@@ -279,6 +279,4 @@ private static ZoneInfoOld toZoneInfoOld(TimeZone tz) throws Exception { | |||
(int[])simpleTimeZoneParams.get(tz), | |||
willGMTOffsetChange.getBoolean(tz)); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch will be cleaner if we remove the line change (L 282-283) as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/integrate |
@johnyjose30 |
/sponsor |
Going to push as commit ebc17c7.
Your commit was automatically rebased without conflicts. |
@coffeys @johnyjose30 Pushed as commit ebc17c7. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Timezone data 2024b changes
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21265/head:pull/21265
$ git checkout pull/21265
Update a local copy of the PR:
$ git checkout pull/21265
$ git pull https://git.openjdk.org/jdk.git pull/21265/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21265
View PR using the GUI difftool:
$ git pr show -t 21265
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21265.diff
Webrev
Link to Webrev Comment