Skip to content
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

8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST #1741

Closed
wants to merge 2 commits into from

Conversation

@naotoj
Copy link
Member

@naotoj naotoj commented Dec 10, 2020

Hello,

Please review the changes to the subject issue. getMinimalDaysInFirstWeek() for Windows has been implemented to suffice the bug claim.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1741/head:pull/1741
$ git checkout pull/1741

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 10, 2020

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2020

@naotoj The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

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.

@naotoj naotoj marked this pull request as ready for review Dec 10, 2020
@openjdk openjdk bot added the rfr label Dec 10, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 10, 2020

Webrevs

Copy link
Member

@JoeWang-Java JoeWang-Java left a comment

Looks good to me. Some minor comments below.

@@ -75,7 +75,7 @@

// CalendarData value types
private static final int CD_FIRSTDAYOFWEEK = 0;
private static final int CD_MINIMALDAYSINFIRSTWEEK = 1;
Copy link
Member

@JoeWang-Java JoeWang-Java Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep the naming consistent, doing the same change to, e.g. the corresponding macosx impl?

Copy link
Member Author

@naotoj naotoj Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constants are for native methods, which differ between macOS and Windows. Thus I thought it would be clearer to align the name with Windows' constants.

Copy link
Member

@JoeWang-Java JoeWang-Java Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense.

removeExtensions(locale).toLanguageTag(),
CD_FIRSTWEEKOFYEAR);
return switch (firstWeek) {
case 1 -> 7;
Copy link
Member

@JoeWang-Java JoeWang-Java Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be good to use constants or enum instead of literal, or maybe at least a note for the case numbers.

Copy link
Member Author

@naotoj naotoj Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be better by making them as enums, but there are other locations similarly using ints. I added extra comments to explain those ints for better readability.

@@ -172,6 +172,9 @@ public static void main(String[] args) throws Throwable {

//testing 8248695 fix.
testRun("HOST", "bug8248695Test", "", "", "");

//testing 8257964 fix. (macOS/Windows only)
testRun("HOST", "bug8257964Test", "", "", "");
Copy link
Member

@JoeWang-Java JoeWang-Java Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test runs only if the platform locale is en-GB. Do we know if the test system run tests on multiple locales? From the console output unfortunately, it's impossible to tell which specific tests were run

Copy link
Member Author

@naotoj naotoj Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be, but it is enough to test one locale to demonstrate platform settings are used for minimal days in first week. Each test case name can be found in the command line to the java launcher which is logged in the .jtr file.

Copy link
Member

@JoeWang-Java JoeWang-Java Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know it's shown in the .jtr file. Let's hope a link to the test result (html) will be provided in the future :-)

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@naotoj 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:

8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST

Reviewed-by: joehw

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 16 new commits pushed to the master branch:

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@naotoj you need to get approval to run the tests in tier1 for commits up until 3f40c3d

@openjdk openjdk bot added the test-request label Dec 11, 2020
@naotoj
Copy link
Member Author

@naotoj naotoj commented Dec 11, 2020

/integrate

@openjdk openjdk bot closed this Dec 11, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@naotoj Since your change was applied there have been 16 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 74b79c6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@naotoj naotoj deleted the JDK-8257964 branch Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants