Re: RFR: JDK-8183579: refactor and cleanup launcher help messages
Mandy, Thanks for the reviewplease see in-lined comments, On Jul 20, 2017, at 11:53 AM, Kumar Srinivasan 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
Re: RFR: JDK-8183579: refactor and cleanup launcher help messages
> On Jul 20, 2017, at 11:53 AM, Kumar Srinivasan > 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
RFR: JDK-8183579: refactor and cleanup launcher help messages
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
