On Thu, 13 Nov 2025 03:17:22 GMT, Alexander Matveev <[email protected]>
wrote:
>> Alexey Semenyuk has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> MainResources.properties: remove unreferenced L10N key
>
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/HelpFormatter.java
> line 46:
>
>> 44: this.optionGroups = Objects.requireNonNull(optionGroups);
>> 45: this.formatter = Objects.requireNonNull(formatter);
>> 46:
>
> Extra new line.
Fixed
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/JOptSimpleOptionsBuilder.java
> line 184:
>
>> 182: try {
>> 183: initializer.run();
>> 184: throw new AssertionError();
>
> Can you put a comment on why we need `throw new AssertionError()`? I assume
> that `initializer.run()` never exits.
Added a comment
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/OptionSpec.java line
> 126:
>
>> 124: * <p>
>> 125: * If the option has three names "a", "b", and "c", the stream will
>> have three
>> 126: * option spec objects each with a single name. The firt will have
>> name "a", the
>
> `firt` -> `first`
Fixed
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardAppImageFileOption.java
> line 227:
>
>> 225: }).map(option -> {
>> 226: var spec = context.mapOptionSpec(option.spec());
>> 227: var strValue =
>> Optional.ofNullable(properties.get(spec.name().name()));
>
> `name().name()` why we need name of the name and what it means?
`OptionSpec.name()` method returns a reference to an object of type
`OptionName` that also has a `name()` method. Option name can be "option", or
"--option", or "-o" string. So I decided to introduce the `OptionName` class to
encapsulate these variants.
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java
> line 492:
>
>> 490: context.asFileSource().ifPresent(propertyFile -> {
>> 491:
>> b.converterExceptionFactory(forMessageWithOptionValueAndName(propertyFile));
>> 492:
>> b.converterExceptionFormatString("error.properties-paramater-not-path");
>
> `paramater` -> `parameter`. Below as well.
Fixed
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java
> line 544:
>
>> 542: }))
>> 543: .converterExceptionFactory(ERROR_WITH_VALUE_AND_OPTION_NAME)
>> 544: //
>> .converterExceptionFormatString("error.paramater-not-launcher-shortcut-dir")
>
> Remove if not needed.
Fixed
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/cli/StandardOption.java
> line 644:
>
>> 642: final var theCause = cause.orElseThrow();
>> 643: if (theCause instanceof AddLauncherSyntaxException)
>> {
>> 644: // return
>> ERROR_WITH_VALUE_AND_OPTION_NAME.create(optionName,
>
> Remove if not needed.
Fixed
> test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/cli/StandardOptionTest.java
> line 403:
>
>> 401: }
>> 402:
>> 403: // Builder expectMalformedError(String v) {
>
> Remove if not needed.
Fixed
> test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/cli/StandardOptionTest.java
> line 544:
>
>> 542: buildLauncherShortcutTest().optionValue("false"),
>> 543: buildLauncherShortcutTest().optionValue(""),
>> 544: //
>> buildLauncherShortcutTest().optionValue("").propertyFile(true),
>
> Remove and line 547.
Fixed
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528828988
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528820042
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528837414
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528844728
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528827278
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528856437
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528856223
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528827729
PR Review Comment: https://git.openjdk.org/jdk/pull/28163#discussion_r2528828212
