On Tue, 29 Sep 2026 20:22:34 GMT, Justin Lu <[email protected]> wrote:
>> sbracely has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update the test configuration properties
>
> test/jdk/java/time/nonjunit/java/time/chrono/HijrahConfigCheck.java line 79:
>
>> 77:
>> 78: // Variant configuration test
>> 79: HijrahChronology variantChronology = (HijrahChronology)
>> Chronology.of("islamic-variant");
>
> I would use the variable for "islamic-variant" that was defined above.
Done, now uses VARIANT_CALTYPE.
> test/jdk/java/time/nonjunit/java/time/chrono/HijrahConfigCheck.java line 82:
>
>> 80: HijrahDate hijrahDateWithVariant = HijrahDate.of(1300, 1,
>> 30).withVariant(variantChronology);
>> 81: HijrahDate expected = variantChronology.date(1300,1,29);
>> 82: if (!hijrahDateWithVariant.equals(expected)) {
>
> I would also add a brief comment describing that the variant only supports 29
> days so it gets clamped, that way a future reader does not have to dig into
> the properties file to figure it out.
Added a comment that the variant month has 29 days, so day 30 is clamped.
> test/jdk/java/time/nonjunit/java/time/chrono/HijrahConfigTest.java line 35:
>
>> 33: * @test
>> 34: * @summary Tests whether a custom Hijrah configuration properties file
>> works correctly
>> 35: * @bug 8187987 8392848
>
> Bug header needs to be updated with the JBS issue.
Updated the bug header to include 8393048
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33079#discussion_r4139023517
PR Review Comment: https://git.openjdk.org/jdk/pull/33079#discussion_r4139022819
PR Review Comment: https://git.openjdk.org/jdk/pull/33079#discussion_r4139030022
