RFR: JDK-8183579: refactor and cleanup launcher help messages

Kumar Srinivasan kumar.x.srinivasan at oracle.com
Fri Jul 28 14:33:57 UTC 2017


Mandy,

Thanks for the review....please see in-lined comments,

>> On Jul 20, 2017, at 11:53 AM, Kumar Srinivasan<kumar.x.srinivasan at oracle.com>  wrote:
>>
>> Hi,
>>
>> Please review refactoring and clean up of the java launcher's help/usage
>> messages.
>>
>> The highlights of the changes are as follows:
>>
>> 1. Helper.java: is renamed from LauncherHelper.java, simpler name,
>>     containing mostly methods to help with class loading, module assistance etc.
>>
> There are tests depending on `sun.launcher.LauncherHelper` class name.
> I actually like LauncherHelper better than Helper to make it clear
> what it is just by its simple name.
The rationale was to simplify the fullname from
sun.launcher.LauncherHelper -> sun.launcher.Helper

wrt. the test Ugh, Ok renamed Helper.java back to LauncherHelper.java

>> Webrev:
>> http://cr.openjdk.java.net/~ksrini/8183579/webrev.00/
> launcher.properties
>    40 # Translators please note do not translate the options themselves
>
> Option names are no longer in the resource bundle.
> It would be helpful to add the comment describing the key name
>    java.launcher.opt.$OPTION_NAME.param
>    java.launcher.opt.$OPTION_NAME.desc

Added the notes/comments describing what needs to be done wrt.
adding any new keys.

> A newline between each option may help readability.
> Since the whitespace in description is irrelevant, maybe keep the indentation
> to 4 space?

Done

> Does the help message show the options in the order listed here?

In the declaration order.

> I would hope it uses the order of the Option enums.  If so, we could
> list them in alphabetical order in launcher.properties.

The keys are alpha sorted now.

>    80 java.launcher.opt.x.desc = print help on extra options to the error stream
>
> Should $OPTION_NAME match the option name (rather than toLowerCase)?

Ok used the Enum name directly with no translations.

> OptionsHelper.java
>   311         Arrays.asList(Option.values()).stream()
>
> Alternative: Stream(Option.values())

Done.

> This class includes the entry point for -XshowSettings but not other options such as —-list-modules.  It may cause confusion to which class a new launcher option is added.  It may be okay.   Maybe just move the code for printing the help messages in Option class and consider the refactoring for -XshowSetting and other options in the future.

As you suggested, moved only the two usage helper methods to the new 
UsageHelper.

This simplifies the changes.

New webrev at:
http://cr.openjdk.java.net/~ksrini/8183579/webrev.01/

Thanks

Kumar

> Mandy



More information about the core-libs-dev mailing list