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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kumar Srinivasan
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.

2. OptionHelper.java: moved those methods in Helper.java, pertaining
     to option printing, to this class.

3. Options.java: is heavily inspired by javadoc's option management, it
     is very structured, and makes the resources much  simpler to manage.

4. Modified the options descriptions to be _more consistent_,
     keeping in mind, these options have been in there for eons.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8183579

Webrev:
http://cr.openjdk.java.net/~ksrini/8183579/webrev.00/

java -help output:
http://cr.openjdk.java.net/~ksrini/8183579/help.txt

java -X output:
http://cr.openjdk.java.net/~ksrini/8183579/x-help.txt

Thanks
Kumar




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mandy Chung

> On Jul 20, 2017, at 11:53 AM, Kumar Srinivasan <[hidden email]> 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.

>
> 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

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

Does the help message show the options in the order listed here?
I would hope it uses the order of the Option enums.  If so, we could
list them in alphabetical order in launcher.properties.

  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)?

OptionsHelper.java
 311         Arrays.asList(Option.values()).stream()

Alternative: Stream(Option.values())

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.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kumar Srinivasan
Mandy,

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

>> On Jul 20, 2017, at 11:53 AM, Kumar Srinivasan<[hidden email]>  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

Loading...