[rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser
Lukasz Dracz
ldracz at redhat.com
Mon Oct 6 21:24:12 UTC 2014
Hello,
> Uff... Now you will hate me. I have changed my mind a bit, And i need we need
> to fix an ols parser a
> bit.
>
> At first inline
>
>
> Rest innline
>
> >
> >
> > itweb-settingsOptionParser-6.patch
> >
> >
> > diff -r 6d62f68fb037 launcher/launchers.in
> > --- a/launcher/launchers.in Mon Sep 22 17:10:07 2014 +0200
> > +++ b/launcher/launchers.in Thu Sep 25 17:26:49 2014 -0400
> > @@ -53,7 +53,7 @@
> > *)
> > ARGS[$j]="$1"
> > j=$((j+1))
> > - if [ "$1" = "-headless" ] ; then
> > + if [[ "$1" =~ -*headless ]] ; then
> > SPLASH="false"
> > fi
> > ;;
>
> Thsi is good. Please push as separate changeset (please let aazores/jie to
> check the changelog
> before push)
>
> > diff -r 6d62f68fb037 netx/net/sourceforge/jnlp/OptionsDefinitions.java
> > --- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java Mon Sep 22 17:10:07
> > 2014 +0200
> > +++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java Thu Sep 25 17:26:49
> > 2014 -0400
> > @@ -73,14 +73,14 @@
> > TRUSTNONE("-Xtrustnone","BOTrustnone"),
> > JNLP("-jnlp","BOJnlp", NumberOfArguments.ONE),
> > //itweb settings
> > - NODASHHELP("help", "IBOHelp"),
> > - LIST("list", "IBOList"),
> > - GET("get", "name", "IBOGet"),
> > - INFO("info", "name", "IBOInfo"),
> > - SET("set", "name value", "IBOSet"),
> > - RESETALL("reset", "all", "IBOResetAll"),
> > - RESET("reset", "name", "IBOReset"),
> > - CHECK("check", "name", "IBOCheck"),
> > + NODASHHELP("-help", "IBOHelp"),
>
> we dont need this ;) This isnow duplication of HELP (maybe only the
> properties message needs tuning
> to suite both - if possible) otherwise wee need to go with some HELP2 (I
> prefer first)
Using only one Help now for both. Changed the message.
> > + LIST("-list", "IBOList"),
> > + GET("-get", "name", "IBOGet", NumberOfArguments.ONE_OR_MORE),
> > + INFO("-info", "name", "IBOInfo", NumberOfArguments.ONE_OR_MORE),
> > + SET("-set", "name value", "IBOSet",
> > NumberOfArguments.ONE_OR_MORE),
> > + RESETALL("-reset", "all", "IBOResetAll"),
> > + RESET("-reset", "name", "IBOReset",
> > NumberOfArguments.ONE_OR_MORE),
> > + CHECK("-check", "name", "IBOCheck"),
>
> Otherwise this is good.
> > //policyeditor
> > //-help
> > FILE("-file", "policy_file", "PBOFile"),
> > @@ -160,8 +160,9 @@
> > OPTIONS.GET,
> > OPTIONS.INFO,
> > OPTIONS.SET,
> > + OPTIONS.RESET,
> > OPTIONS.RESETALL,
> > - OPTIONS.RESET,
> > + OPTIONS.HEADLESS,
> > OPTIONS.CHECK
> > });
> > }
> > diff -r 6d62f68fb037
> > netx/net/sourceforge/jnlp/controlpanel/CommandLine.java
> > --- a/netx/net/sourceforge/jnlp/controlpanel/CommandLine.java Mon Sep 22
> > 17:10:07 2014 +0200
> > +++ b/netx/net/sourceforge/jnlp/controlpanel/CommandLine.java Thu Sep 25
> > 17:26:49 2014 -0400
> > @@ -21,8 +21,6 @@
> > import static net.sourceforge.jnlp.runtime.Translator.R;
> >
> > import java.io.IOException;
> > -import java.util.ArrayList;
> > -import java.util.Arrays;
> > import java.util.List;
> > import java.util.Map;
> >
> > @@ -37,16 +35,17 @@
> > import net.sourceforge.jnlp.util.docprovider.TextsProvider;
> > import
> > net.sourceforge.jnlp.util.docprovider.formatters.formatters.PlainTextFormatter;
> > import net.sourceforge.jnlp.util.logging.OutputController;
> > +import net.sourceforge.jnlp.util.optionparser.OptionParser;
> >
> > /**
> > * Encapsulates a command line interface to the deployment configuration.
> > * <p>
> > - * The central method is {@link #handle(String[])}, which calls one of the
> > - * various 'handle' methods. The commands listed in {@link #allCommands}
> > are
> > + * The central method is {@link #handle()}, which calls one of the
> > + * various 'handle' methods. The commands listed in OptionsDefinitions are
> > * supported. For each supported command, a method handleCOMMANDCommand
> > exists.
> > * This method actually takes action based on the command. Generally, a
> > * printCOMMANDHelp method also exists, and prints out the help message
> > for
> > - * that specific command. For example, see {@link
> > #handleListCommand(List)}
> > + * that specific command. For example, see {@link #handleListCommand()}
> > * and {@link #printListHelp()}.
> > * </p>
> > * Sample usage:
> > @@ -72,6 +71,7 @@
> >
> >
> > DeploymentConfiguration config = null;
> > + private static OptionParser optionParser;
> >
> > /**
> > * Creates a new instance
> > @@ -125,11 +125,11 @@
> > /**
> > * Handles the 'list' command
> > *
> > - * @param args the arguments to the list command
> > * @return result of handling the command. SUCCESS if no errors
> > occurred.
> > */
> > - public int handleListCommand(List<String> args) {
> > - if (args.contains("help")) {
> > + public int handleListCommand() {
> > + List<String> args =
> > optionParser.getValues(OptionsDefinitions.OPTIONS.LIST);
> > + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.NODASHHELP))
> > {
> > printListHelp();
> > return SUCCESS;
> > }
> > @@ -169,33 +169,29 @@
> > /**
> > * Handles the 'get' command.
> > *
> > - * @param args the arguments to the get command
> > * @return an integer representing success (SUCCESS) or error
> > handling the
> > * get command.
> > */
> > - public int handleGetCommand(List<String> args) {
> > - if (args.contains("help")) {
> > + public int handleGetCommand() {
> > + List<String> args =
> > optionParser.getValues(OptionsDefinitions.OPTIONS.GET);
> > + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.NODASHHELP))
> > {
> > printGetHelp();
> > return SUCCESS;
> > }
> >
> > - if (args.size() != 1) {
> > - printGetHelp();
> > - return ERROR;
> > - }
> > -
> > Map<String, Setting<String>> all = config.getRaw();
> >
> > - String key = args.get(0);
> > - String value = null;
> > - if (all.containsKey(key)) {
> > - value = all.get(key).getValue();
> > - OutputController.getLogger().printOutLn(value);
> > - return SUCCESS;
> > - } else {
> > -
> > OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
> > R("CLUnknownProperty", key));
> > - return ERROR;
> > + for (String key : args) {
> > + String value = null;
> > + if (all.containsKey(key)) {
> > + value = all.get(key).getValue();
> > + OutputController.getLogger().printOutLn(key+": "+value);
> > + } else {
> > +
> > OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
> > R("CLUnknownProperty", key));
> > + return ERROR;
> > + }
> > }
> > + return SUCCESS;
> > }
> >
> > /**
> > @@ -210,39 +206,46 @@
> > /**
> > * Handles the 'set' command
> > *
> > - * @param args the arguments to the set command
> > * @return an integer indicating success (SUCCESS) or error in
> > handling
> > * the command
> > */
> > - public int handleSetCommand(List<String> args) {
> > - if (args.contains("help")) {
> > + public int handleSetCommand() {
> > + List<String> args =
> > optionParser.getValues(OptionsDefinitions.OPTIONS.SET);
> > + args = OptionParser.splitListOnEquals(args);
> > + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.NODASHHELP))
> > {
> > printSetHelp();
> > return SUCCESS;
> > }
> >
> > - if (args.size() != 2) {
> > + if (args.size() % 2 != 0) {
>
> This is no go. This must be handeld by parser.
> on optionParser.getValues(OptionsDefinitions.OPTIONS.SET);
> parser must return you something "known"
>
> your choice if it is some special object of you,which contains key+value, or
> values separated by =,
> or always even numer of args (probably best). NBUt parser is who have to
> prepare it - and check
> it. (==die when non even result is at the end or whatever)
Okay. With this I added a new exception called UnevenParameterException, that is thrown when even pairs are expected but not found. When the order matters, it ensures each command that expects even pairs has even pairs each occurrence where as if order does not matter then it checks that it has even pairs for the total params of the command.
Order Matters
Ex. evenExpectingCommand param1 param2 OTHERCOMMAND evenExpectingCommand param3 param4
OKAY
Ex. evenExpectingCommand param1 param2 param3 OTHERCOMMAND evenExpectingCommand param4 param5 param 6
NOT OKAY throws UnevenParameterException
Not Order Matters
Ex. evenExpectingCommand param1 param2 OTHERCOMMAND evenExpectingCommand param3 param4
OKAY
Ex. evenExpectingCommand param1 param2 param3 OTHERCOMMAND evenExpectingCommand param4 param5 param 6
OKAY
Ex. evenExpectingCommand param1 OTHERCOMMAND evenExpectingCommand param4 param5
NOT OKAY throws UnevenParameterException
> > printSetHelp();
> > return ERROR;
> > }
> > + String key = "";
> > + String value;
> >
> > - String key = args.get(0);
> > - String value = args.get(1);
> > -
> > - if (config.getRaw().containsKey(key)) {
> > - Setting<String> old = config.getRaw().get(key);
> > - if (old.getValidator() != null) {
> > - try {
> > - old.getValidator().validate(value);
> > - } catch (IllegalArgumentException e) {
> > -
> > OutputController.getLogger().log(OutputController.Level.WARNING_ALL,
> > R("CLIncorrectValue", old.getName(), value,
> > old.getValidator().getPossibleValues()));
> > - OutputController.getLogger().log(e);
> > - return ERROR;
> > + for (String arg : args) {
> > + if (args.indexOf(arg) % 2 == 0) {
>
> same
> > + key = arg;
> > + } else {
> > + value = arg;
> > + if (config.getRaw().containsKey(key)) {
> > + Setting<String> old = config.getRaw().get(key);
> ... snip. simialr issues.
>
> > + assertEquals("baseballbat", list.get(6));
> > + }
> > +
> > }
> > \ No newline at end of file
> >
>
>
> Well what crossed my mind.
>
> at first - you must hanlde your set key=value in parser - that means new
> NUmberOfArguments value.
> Some EVEN_NUMBER_OR_WITHEQUALCHAR. ok?
>
>
> And then parser will figh tiwith it.
>
>
> Then I was thinking about the multiple options versus limited options.
>
> To introduce limited options, it really need new parameter into
> OPTIONS(String option, String helperString, String decriptionKey,
> NumberOfArguments
> numberOfArguments) {
> not instead NumberOfArguments, you got me wrong. *new*.
>
> So it will be OPTIONS(String option, String helperString, String
> decriptionKey, NumberOfArguments
> numberOfArguments, ArgumentOccurence oc) {
> where ArgumentOccurence is new enum with values like exactly one, one or
> more, unique.. or
> similar. Eg Xclearcahce is good candidate for NON_OR_UNIQUE (as it have no
> sensein other cases...
> And I'm still not sure with how to proceed with HELP - it clearly cannot be
> NONE_OR UNIQUE, but
> should be :D )
>
> But his would need to be tuned as separate changeset *before* tthis patch (as
> it is independent
> patch toOptionPArser)
>
> However, as javaws have 99% of arguments asked like hasOption() (except
> -arg,-param -property", -update and -jnlp (and one null - the main arg)
> and those args are handled in main in order, and javaws is terinaed
> accordingly, and... well this
> sucks - only *first* of them have actually effect.
>
> The second one is silently ignored (?) - except mainarg - in this case javaws
> is terminated.
>
> On one side, it is really god to have so powerfull validation, on seconf,
> maybe it is to enforcing -
> thoughts?
>
> So I .. once javaws is so lenient to params, why not itweb-settings.
Yeah
> And, with your original idea of allmighty interpreter - it will be even
> *simple*
>
> So my idea was.
>
> You have
> private final Map<OptionsDefinitions.OPTIONS, List<String>>
> parsedOptions;
> Lets change it to list!
> private final List<ParsedOption>
>
> where parsed option is NewClass
> contaiing
> OptionsDefinitions.OPTIONS, name
> and List<String> params
>
> (dont forget about null mian param, and param-les soptions)
> - you may also move some(all?) verification (on number of params) logic to
> this class
>
> Now - you wil have to change hasOption method - but afaik it is all.
>
> In this parsedOptions you naow have all items, including the order. And of
> course validated values
> (both valid options only, and valid params only)
>
> So the itweb-stting cmd clinet, should do nothhing more, then iterate through
> this array, and
> evaluate each param....
>
>
> Now - the best solution is probably *good* mixture of ArgumentOccurence and
> lanient list...I jsut
> can not see it.
>
>
>
> I'm on pto next week, please consult with jie/andrew/jacob/omair. Maybe in
> meantime you may adapt
> PolicyEditor to parser - it is much simple. But I would liek to verify tthe
> itweb-setting option
> parser before push. And of course you may discuse those list x
> ArgumentOccurence +/-
>
>
> Sorry for neverending itterations, but I'm still somehow unhappy about the
> state of this future.
It's alright, I want to get this into a great state too.
I implemented your ParsedOption idea, which I think is really good. I also changed it back to allowing Multiple Options again instead of limited options so I didn't implement ArgumentOccurence. Should we go with Multiple Options being allowed or limited options ? I think with the List of ParsedOption approach, multiple option is implemented better than before. Also the main help message is displayed only if help is the first command or after headless as the 2nd command, any help after that will count display help for the command before it, which means you could display multiple helps ex. ./itweb-settings get help set help reset help will show the command help for get set reset.
Also now the static splitListOnEquals/Symbols is no longer used/needed other than the unit tests, Should I remove them ? (I'm of the opinion yes).
Thank you,
Lukasz Dracz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itweb-settingsOptionParser-10.patch
Type: text/x-patch
Size: 60783 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141006/114c69cb/itweb-settingsOptionParser-10-0001.patch>
More information about the distro-pkg-dev
mailing list