[rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser

[Lukasz Dracz]

Hello,

I have changed the parser to take into account the order of options it receives. For itweb-settings it goes through the options in order, so the order in which the commands are issued matter. Ex. 'itweb-settings get deployment.log.file set deployment.log.file true get deployment.log.file reset deployment.log.file get deployment.log.file' will go through in order with the first get then set it to true, show you it is true with get then reset it and use get after the reset. Also now help can be combined with options so 'itweb-settings get help set deployment.log.file true' will show the help for get and do the specified set option. This does mean to get the big main help message the help command has to be present right after itweb-settings or you could do itweb-settings headless help.

> Mayor hints
>   - when resetAll is presented - then no metter of other arguments, the
>   processing have tobe
> stopped, and user warned - Something like "resetAl found. If you wont to use
> this comamnd, use it
> alone. Otherwise reomvoe it from comamndline". And halt.

I have changed how it works, and don't agree with reset all not being allowed to use with other commands.
For ex. Someone might want to see what a command is before they reset everything so they know what to set it to after
'itweb-settings get deployment.log.file reset all'
OR they want exactly one setting to be changed apart from the default so something like
'itweb-settings reset all set deployment.security.level=ALLOW_UNSIGNED'
I think now that order to the options is possible, allowing reset all with other commands is fine.
 
>   - the docs must be adjusted
>     - description of all affected OptionDefinitions in messages. properties.
>     - general documentation to itweb=settings commandline (can be done as
>     separate changeset)
>     - there is bug in check's docs - it stays it need parameter what to check
>   - there is (unrelated to your changset) "bug" in check - it is ruing
>   validators, and validators do
> (if application is running under X ) poping joption pane - maybe we can add
> hedless support ? (afaik
> only need is to JNLPruntime.setHeadless(parser.has(HEADLESS) ) - but needs
> testing

Added a check for headless as you suggested. Also changed launchers.in as needed.

> - how does it behave  when multiple combinations of reset,set,get,info are
> presented?
> - what about some  cornercase like set property=value get something set
> p.t=abc reset aaa get aaa
> set aaa=bbbb ....
>     - I would expect it will set proeprty to value, prints out vlae of
>     something, set p.t to abc,
> reset aaa to defaul and set aaa to bbbb

With the new order it should do this.

>      - it does not seem to do this:(
>      - after your patch - it does only set proeprty to value, prints out vlae
>      of something. The
> second set would never be executed. right?
>      - imho the safest way is to allow only one of set | reset | get | chec..
>      And if more of thsoe
> is present.Simply die. Is it even possibele with your current
> parserimplementation to recognize two
> identical options presented two times? If no, then stop execution is the only
> way:(

It wasn't possible, but I updated OptionParser to make this possible.
I think allowing multiple set | reset | get | check is fine now.

> 
> - there is significant change in behaviour
>     - before it was accepting set name value
>     - now it seems to me to accept set name=value, or not?
>   - considering the nature of purpos of this tool, I would stay withold
>   approach, or with both. Not
> only with new ;(
> 

Yes I added support for set name=value because that is how they are stored in deployment.properties so I thought a user seeing the file might want to set them in the same manner. It also works in the old way where you place a space between name and value. I thought it would make it easier to use, I don't foresee the issue of "name" having a "=" in it as a legitimate property. Yes something like na=me=value if 'na=me' was a legitimate property would read the propery as na with value me and value would be the next property seen. If you think name might have equals / there is another issue I will change it back, I just thought this would make it easier for the user.

> 
> Here is valid same issue as in your first opton parser. What is value
> consists of symbol?
> param=va=lu=e will be broken :(

I could make it just break on the first equals it finds. Would that be more desirable behaviour ? So in this case set param=va=lu=e would be the same as set param va=lu=e.

> 
> But this will be valid only if we agree on bothold and new style. If we agree
> only on old one those
> will go out:(
> 
> Anyway - nitpickers nit - tey both should be static and tested :)
> > +
> > +    public List<String> getValuesSplitOnEquals(OptionsDefinitions.OPTIONS
> > option) {
> > +        return getValuesSplitOn(option, "=");
> > +    }
> > +
> > +    public int getNumberOfOptions() {
> > +        if (mainArgExists()) {
> > +            return parsedOptions.size() - 1;
> > +        }
> > +        return parsedOptions.size();
> > +    }
> > +
> >   }
> >

Made these two static and added unit tests for them as well as the other new methods added.

> int to two "unrelated" bugs - both shoud be fixe din this changeset - as tis
> is mayor rework of the
> inpur handling.
> 
> 
> Thak you!
> 
> (well compared to this, th epolicyeditor will be children's palyground for
> you :) )

I guess so :)

Thank you,
Lukasz Dracz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itweb-settingsOptionParser-5.patch
Type: text/x-patch
Size: 31494 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140922/8d03c37e/itweb-settingsOptionParser-5-0001.patch>