[rfc][icedtea-web] Option Parser

[Jiri Vanek]

On 09/09/2014 05:28 PM, Lukasz Dracz wrote:
> Hello,
>
> Here is the updated patch that applies onto the push that added OptionsDefinitions. I have also added a few more unit tests and refactored a common check in two methods into a third method in the OptionParser. Also changed it so now the Option Parser accounts for the case where the user specifies a dash or not in front of the option, ex. 'command -arg blue yellow green -headless' will return the same result as 'command arg blue yellow green headless'.
>
> Thank you,
> Lukasz Dracz
>
>
> optionParser-12.patch

I'm quite happy with the patch.

Still few nits (really nits) and two questions

  Q1 - your patch tolerates more then one unknown parameter. This should be handled differrently.
  Q2 - By accident I found
BOJnlp      = Location of JNLP file to launch (url or file).
   in message.properties

Now it is not supported.  is it worthy of support?


A2 - I dont know ;) I'm in temptation to return this param. If not, then it shold be removed from 
properties.

A1 depends on A2 :)
  As it is now, if more then one main paramis found, then ITW should die. And print out that last 
parameter is expected to be jnlp file, and all those unknown params were found. (in error_all new 
LunchException)
  If we agree that Q2 answer is yes, then except above reaction there should be second one.
   If -jnlp is s found, then  no unknown param is tolerated, and you can simply die. Anyway your 
parser ust live also with no -jnlp as is now.
>
>
> diff -r b0690979967f netx/net/sourceforge/jnlp/ParserSettings.java
> --- a/netx/net/sourceforge/jnlp/ParserSettings.java	Tue Sep 09 14:51:16 2014 +0200


> -    public static ParserSettings setGlobalParserSettingsFromArgs(String[] cmdArgs) {
> -        List<String> argList = Arrays.asList(cmdArgs);
> -        boolean strict = argList.contains("-strict");
> -        boolean malformedXmlAllowed = !argList.contains("-xml");
> -
> -        globalParserSettings = new ParserSettings(strict, true, malformedXmlAllowed);
> +    public static ParserSettings setGlobalParserSettingsFromArgs(boolean strict, boolean xml) {
> +        globalParserSettings = new ParserSettings(strict, true, !xml);
>           return globalParserSettings;

Get rid of this factory method compeltely.

you can simply
ParserSettings settings =  new 
ParserSettings(optionParser.hasOption(OptionsDefinitions.OPTIONS.STRICT),optionParser.hasOption(OptionsDefinitions.OPTIONS.XML));

in smilar way as you have it now, or not?

>       }
>
> diff -r b0690979967f netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java	Tue Sep 09 14:51:16 2014 +0200

...SNIP...

> +    }
> +
> +    private boolean stringEqualsOption(String s, OptionsDefinitions.OPTIONS opt) {

Ouch this can be done much nicer...
Just strip all leading "-" and all behind "="we are done.
then you have only one simple s.equals[IgnoreCase?]

> +        if (s.equals(opt.option) || s.equals("-".concat(opt.option)) || s.split("=")[0].equals(opt.option)
> +                || s.split("=")[0].equals("-".concat(opt.option)) || "-".concat(s).equals(opt.option) || "-".concat(s).equals("-".concat(opt.option))
> +                || "-".concat(s).split("=")[0].equals(opt.option) || "-".concat(s).split("=")[0].equals("-".concat(opt.option))) {
> +            return true;
> +        }

Except above, its a msut that iweb-settings and policyeditor uses this option parser to.

You may do it as different changeset but you have to. Mybe evennow is goo dtime to check if your 
parser will suit.


Thank you!

  J.