[rfc][icedtea-web] Option Parser

[Lukasz Dracz]

Hello,

Here is the updated patch based on most of your suggestions. 

>Just to be clear, this patch changes behaviour too, right?

I suppose your right, I tried my best not to, but some parts were changed.

>>      private static final String doubleArgs = "-basedir -jnlp -arg -param -property -update";
>Is this variable used any more?

It was in getJNLPFile but I rewrote that part too in this updated patch. So it is gone now.

>Code like this is a hint that maybe you should have called your class ArgumentParser

Haha you are right, but since I added Option interface and OptionParser Config enum, I changed the name to optionParser instead.

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

Thank you ! checkOption looked weird to me as well at the time but I couldn't think of anything that wasn't too verbose (checkForOption, checkForTheOption :P). 

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

Added better comment.

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

I did not notice it at the time either, changed :)

>You need to get rid of doubleArgs,

Gone, had to rewrite getJNLPFile.

> and of thet terrible Enum.

Your not going to like me, I kept the Enum but I put it somewhere else.

Why is it a terrible Enum ?

>I would suggest you to change
>public OptionParser(String[] args)
>
>to
>
>public OptionParser(String[] args, OptionParserConfig cfg)

Good idea, done.

Jiri, I implemented it a little differently than you suggested, and was wondering what your thoughts on it are. (as well as Omair's, Jie and anyone else's too :) ) I made an enum that holds the config you decide to use that will be passed into the Option Parser. The enum you choose references an Enum which holds all the possible options for the config (that must implement interface Option to be compatible with the Option Parser). If you feel it should still be implemented more in the way you suggested, I will convert it to work in that way.

Also I have Placeholder Enums for ITWSettings and PolicyEditor Options as I will have a look into those later. I will not push those, but add them in a later patch when/if they are done.

Thanks for the suggestions Omair, Jiri and Jie ! 

Regards,
Lukasz Dracz 




-------------- next part --------------
A non-text attachment was scrubbed...
Name: OptionParser-5.patch
Type: text/x-patch
Size: 33843 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140825/e2048ea0/OptionParser-5-0001.patch>