[rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser
Jiri Vanek
jvanek at redhat.com
Fri Sep 19 14:11:33 UTC 2014
On 09/18/2014 11:28 PM, Lukasz Dracz wrote:
> Hello,
>
> I have worked on changing itwebsettings to use the Option Parser.
It odes not :) It changes the argument recognition of itweb-settings completely....
Although this change may be dangerous, I like it.
> The biggest change this code brings is the ability to do multiple itwebsettings options at once, you can for example
> set and get or reset and set (reset happens first then set is applied). Also I have reworked get, info, set, and reset to allow to take in multiple args instead of just one. Ex. 'itweb-settings get deployment.security.level deployment.manifest.attributes.check deployment.console.startup.mode' will get all three.
>
> itweb-settings help will display the main help, where as itwebsettings get help will display the specific help for get. If help is present and other options are present it will get the help for all the other options Ex. itweb-settings get help reset check will get the help for (get, reset and check). A limitation is you can't do for ex. 'itweb-setting get help reset all info deployment.javapi.cache.enabled' this will simply display help for get, reset and info only. I can look into getting it to work by displaying help for get, resetting all and getting the info for deployment.javapi.cache.enabled, but then I think I am going to have to limit the help to appear right after the command which would lead to for example, itweb-settings get help reset help info help to get help for all. I'm just wondering if anyone has an opinion on this matter of what would be the preferred way to handle this.
>
> Also looking at the main help that is outputted 'check name - Checks that the current value of the setting is a valid value.' that description is not accurate of what happens in the code. The check option checks through all the values and outputs those that are wrong. Should I implement it to work by just checking the named value if available and just check all if nothing is specified or should the help message text be changed ? Personally I think it is fine as it is and the help message text should just be changed to how it works.
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.
- 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
- zzz 10minutel alter - nother bug - once reset(all) is caled - then it runs validators on old
values.. whyyyy?
- 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
- 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:(
- 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 ;(
little bit more inline
>
> Thank you,
> Lukasz Dracz
>
>
> itwebsettingsOptionParser-2.patch
>
>
> diff -r e1c7156ed0a1 netx/net/sourceforge/jnlp/OptionsDefinitions.java
> --- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java Thu Sep 18 13:18:11 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java Thu Sep 18 17:01:32 2014 -0400
> @@ -75,11 +75,11 @@
> //itweb settings
> NODASHHELP("help", "IBOHelp"),
> LIST("list", "IBOList"),
> - GET("get", "name", "IBOGet"),
> - INFO("info", "name", "IBOInfo"),
> - SET("set", "name value", "IBOSet"),
> + 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"),
> + RESET("reset", "name", "IBOReset", NumberOfArguments.ONE_OR_MORE),
> CHECK("check", "name", "IBOCheck"),
> //policyeditor
> //-help
> @@ -160,8 +160,8 @@
> OPTIONS.GET,
> OPTIONS.INFO,
> OPTIONS.SET,
> + OPTIONS.RESET,
> OPTIONS.RESETALL,
> - OPTIONS.RESET,
> OPTIONS.CHECK
> });
> }
> diff -r e1c7156ed0a1 netx/net/sourceforge/jnlp/controlpanel/CommandLine.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/CommandLine.java Thu Sep 18 13:18:11 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/controlpanel/CommandLine.java Thu Sep 18 17:01:32 2014 -0400
> @@ -37,16 +37,18 @@
> 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;
> +import sun.net.dns.ResolverConfiguration;
You left unused imports here.
>
> /**
> * 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:
...
> - val = handleCheckCommand(arguments);
> - } else {
> - OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, R("CLUnknownCommand", command));
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.SET)) {
> + val = handleSetCommand();
> + }
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.GET)) {
> + val = handleGetCommand();
> + }
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.INFO)) {
> + val = handleInfoCommand();
> + }
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.CHECK)) {
> + val = handleCheckCommand();
> + }
Oooooou.. I like this so much...
> + if (optionParser.mainArgExists()) {
> + OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, R("CLUnknownCommand", optionParser.getMainArgs()));
> handleHelpCommand();
> val = ERROR;
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.NODASHHELP) && optionParser.getNumberOfOptions() == 1) {
> + val = handleHelpCommand();
> }
>
> return val;
> @@ -467,11 +461,13 @@
> */
> public static void main(String[] args) throws Exception {
> DeploymentConfiguration.move14AndOlderFilesTo15StructureCatched();
> + optionParser = new OptionParser(args, OptionsDefinitions.getItwsettingsCommands());
> +
> if (args.length == 0) {
> ControlPanel.main(new String[] {});
> } else {
> CommandLine cli = new CommandLine();
> - int result = cli.handle(args);
> + int result = cli.handle();
>
> // instead of returning, use JNLPRuntime.exit() so we can pass back
> // error codes indicating success or failure. Otherwise using
> diff -r e1c7156ed0a1 netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java
> --- a/netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java Thu Sep 18 13:18:11 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java Thu Sep 18 17:01:32 2014 -0400
> @@ -83,12 +83,20 @@
> } else {
> lastOption = args[i];
> if (parsedOptions.keySet().contains(getOption(lastOption))) {
> +
> if (getOption(lastOption).hasOneOrMoreArguments()) {
> - result = new ArrayList<>(parsedOptions.get(getOption(lastOption)));
> +
> + if (parsedOptions.get(getOption(lastOption)) == null) {
> + result = new ArrayList<>();
> + } else {
> + result = new ArrayList<>(parsedOptions.get(getOption(lastOption)));
> + }
> }
> + } else {
> + parsedOptions.put(getOption(lastOption), null);
> }
> +
> if (getOption(lastOption).hasNoArguments()) {
> - parsedOptions.put(getOption(lastOption), null);
> lastOption = MAINARG;
> }
> }
> @@ -214,4 +222,30 @@
> }
> return new ArrayList<>();
> }
> +
> + public List<String> getValuesSplitOn(OptionsDefinitions.OPTIONS option, String symbol) {
> + List<String> values = new ArrayList<>();
> + for (String arg : getValues(option)) {
> + if (arg.contains(symbol)) {
> + for (String val : arg.split(symbol)) {
> + values.add(val);
> + }
> + } else {
> + values.add(arg);
> + }
> + }
> + return values;
> + }
Here is valid same issue as in your first opton parser. What is value consists of symbol?
param=va=lu=e will be broken :(
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();
> + }
> +
> }
>
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 :) )
J.
More information about the distro-pkg-dev
mailing list