[rfc][icedtea-web] Option Parser

[Jie Kang]

----- Original Message -----
> Hi,
> 
> This is great!
> 
> * Lukasz Dracz <ldracz at redhat.com> [2014-08-22 10:45]:
> > I refactored the way Boot parses arguments by creating an Option
> > Parser to use.
> 
> Just to be clear, this patch changes behaviour too, right? If so, it's
> not completely fair to call it a refactor. From reading the patch the
> following invocation is treated differently before and after:
> 
>     javaws -arg -update /path/to/some.jnlp
> 
> > The reason is that this code could be reused in other
> > parts, instead of writing duplicate code. I have also added unit tests
> > for the parser.
> 
> There's a bit of a problem here. Before this patch, one class knew
> everything about options (what options are possible, what their help
> messages are, what options take an argument) but now two classes have
> this information spread out.
> 
> Have you thought about renaming the OptionParser class to Options or
> something and moving more information about options in there? Or maybe
> OptionParser should not know anything about options and a third class
> that creates OptionParser should know all about options (and other
> option-related information) and tell OptionParser what it needs to use?


Hello,

For some background info this OptionParser is built upon my patch for Localized Man Pages here: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-July/028492.html

Lukas has agreed to take over the enhancement :D


To add to Omair's suggestions, originally I intended for the users of the OptionParser to pass in their own information for the available Options. That way, each user would define their own set of options to be parsed following some standard that we set. I think Omair's suggestion about adding a class, or classes to reduce the responsibility of the Parser is definitely a good idea.



Also small typo nit that I think came from my own mistake x_x:

+    /**
+     * Return all the values of the specified option, or an empty
+     * array if the option is not present.  If the option is a
+     * is present with no parameters then the option name is
+     * returned once.
+     */


Regards,

> 
> > +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java	Fri Aug 22 10:22:15 2014
> > -0400
> 
> >      private static final String doubleArgs = "-basedir -jnlp -arg -param
> >      -property -update";
> 
> Is this variable used any more?
> 
> > +        argsParser = new OptionParser(argsIn);
> 
> Code like this is a hint that maybe you should have called your class
> ArgumentParser :)
> 
> > -        if (null != getOption("-headless")) {
> > +        if (argsParser.checkOption(Option.HEADLESS)) {
> 
> Would it be easier to read this code if checkOption was renamed to
> hasOption? I can't quite figure out (just by looking at the method name)
> what check it is performing.
> 
> > +++ b/netx/net/sourceforge/jnlp/util/OptionParser.java	Fri Aug 22 10:22:15
> > 2014 -0400
> 
> > +//Parses options (generally from main(String[] args)
> > +public class OptionParser {
> 
> I am not a fan of comments, but in cases like this, it is probably a
> good idea to comment on the style of options it parses (--style or
> -style). Maybe also add a note for developer on how to add support for
> more options?
> 
> > +++ b/tests/netx/unit/net/sourceforge/jnlp/util/OptionParserTest.java	Fri
> > Aug 22 10:22:15 2014 -0400
> 
> Thanks for writing these awesome unit tests!
> 
> Thanks,
> Omair
> 
> --
> PGP Key: 66484681 (http://pgp.mit.edu/)
> Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681
> 

-- 

Jie Kang