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