[rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser
Jiri Vanek
jvanek at redhat.com
Tue Oct 21 14:45:23 UTC 2014
On 10/20/2014 04:43 PM, Lukasz Dracz wrote:
> Hello,
>
> I believe I managed to simplify this patch, thanks to jkang's helpful suggestions.
>
>> Please split the patch to three parts
>> - first - add support for the list ParsedOption
>> - second - add support for the equls chars
>> - third - integrate it into itweb settings
>> - fourth - policy editor.
>>
After chat on IRC:
- please return 0-n hyphens. It is really god feature
- please return the EVEN_NUMBER_OR_WITHEQUALCHAR on long run.
- that means - it is ok to remove it now, but for itweb settings I think it will be necessary. Or to drop the set name=val syntaxe, which I will be missing.
- the rmeoval of QUALS_CHAR can be dropepd, as it is default behaviour of parser now (to consider arg val same as arg=val)
Except that the quality of code improved
More in IRC log :)
Thank you both!
J.
>
> This patch adds support for the ParsedOption list, and also the parser has been changed to be simpler.
> Notable changes are that options require a hyphen in front of them, ex. -headless OKAY headless NOT OKAY any more.
> The placement of main arguments is not as flexible as before, it cannot come after an option with many arguments,
> but after option with no args or option with just one arg is okay.
> Unit tests changed to reflect that.
>
> Also from OptionsDefinitions removed EQUALSCHAR and EVEN_NUMBER_Or_WITHEQUALSCHAR enums. The option=arg functionality is still present but does not utilize or plan to use EQUALSCHAR.
>
> Also edited the NEWS and changed back the launcher splashscreen to what it was before.
>
> Thoughts on he changes ?
>
> Thank you,
> Lukasz Dracz
>
> ----- Original Message -----
>> From: "Jiri Vanek" <jvanek at redhat.com>
>> To: "Lukasz Dracz" <ldracz at redhat.com>
>> Cc: "IcedTea" <distro-pkg-dev at openjdk.java.net>
>> Sent: Wednesday, October 8, 2014 5:34:10 AM
>> Subject: Re: [rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser
>>
>> On 10/07/2014 11:49 PM, Lukasz Dracz wrote:
>>> Hello,
>>>
>>>>> 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.
>>>>
>>>> "main help message is displayed only if help is the first command or after
>>>> headless as t"
>>>>
>>>> Cant it be done better?
>>>
>>> Yes,
>>> What if we make it that if only help is put in then it displays the main
>>> help but if help is put with any other combination of commands (get,
>>> reset, check, etc.) it will display the help for those commands (also
>>> disregarding what the command wants) so ex. get deployment.security.level
>>> set help reset, will just get help for get, set and reset ? (and won't
>>> actually display deploymentsecurity.level)
>>>
>>
>> I would say go with simplest(+ simpelst code) possible solution
>>
>>>> well - it does not meter when headless is decalred - you only ask
>>>> "optionPArser.hasOption(headles):
>>>> and set JnlpRuntime accordingly.
>>>>
>>>> the help should decide whether it is global one, or command one more
>>>> cleverly.
>>>>>
>>>>> 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).
>>>>
>>>> Move it to test file then.
>>>
>>> Going to just delete it since I meant the unit tests that use it were ones
>>> that were specifically testing those two methods. I was unclear in my
>>> phrasing sorry :)
>>>
>> oook.
>>
>>>>>
>>>>> Thank you,
>>>>> Lukasz Dracz
>>>>>
>>>>>
>>>>> itweb-settingsOptionParser-10.patch
>>>>>
>>>>>
>>>>> diff -r 8071a44fe6de netx/net/sourceforge/jnlp/OptionsDefinitions.java
>>>>> --- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java Fri Oct 03
>>>>> 14:20:40
>>>>> 2014 -0400
>>>>> +++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java Mon Oct 06
>>>>> 17:16:18
>>>>> 2014 -0400
>>>>> @@ -73,14 +73,13 @@
>>>>> 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"),
>>>>> + 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.EVEN_NUMBER_OR_WITHEQUALCHAR),
>>>>> + RESETALL("-reset", "all", "IBOResetAll"),
>>>>> + RESET("-reset", "name", "IBOReset",
>>>>> NumberOfArguments.ONE_OR_MORE),
>>>>> + CHECK("-check", "name", "IBOCheck"),
>>>>> //policyeditor
>>>>> //-help
>>>>> FILE("-file", "policy_file", "PBOFile"),
>>>>> @@ -123,6 +122,8 @@
>>>>> return numberOfArguments == NumberOfArguments.EQUALS_CHAR;
>>>>> }
>>>>>
>>>>> + public boolean hasEvenNumberOrWithEqualsChar() { return
>>>>> numberOfArguments == NumberOfArguments.EVEN_NUMBER_OR_WITHEQUALCHAR; }
>>>>> +
>>>>> public boolean hasOneOrMoreArguments() {
>>>>> return numberOfArguments == NumberOfArguments.ONE_OR_MORE;
>>>>> }
>>>>> @@ -140,7 +141,8 @@
>>>>> NONE("No argument expected"),
>>>>> ONE("Exactly one argument expected"),
>>>>> ONE_OR_MORE("Expected one or more arguments"),
>>>>> - EQUALS_CHAR("Expected -param=value vaue declaration");
>>>>> + EVEN_NUMBER_OR_WITHEQUALCHAR("Expected one or more arguments
>>>>> with
>>>>> param=value as valid argument"),
>>>>
>>>> this is micro nit, but wgy it was inserted, and not jsut added?
>>>
>>> I have no idea, I probably accidently added a space somewhere in there, now
>>> its added :)
>>>
>>>>
>>>> Also - this hunk no longer applies, as I pushed the localization for this
>>>> class.
>>>>> + EQUALS_CHAR("Expected -param=value value declaration");
>>>>>
>>>>> String messageKey;
>>>>>
>>>>> @@ -155,13 +157,14 @@
>>>>>
>>>>> public static List<OPTIONS> getItwsettingsCommands() {
>>>>> return Arrays.asList(new OPTIONS[]{
>>>>> - OPTIONS.NODASHHELP,
>>>>> + OPTIONS.HELP,
>>>>> OPTIONS.LIST,
>>>>> OPTIONS.GET,
>>>>> OPTIONS.INFO,
>>>>> OPTIONS.SET,
>>>>> + OPTIONS.RESET,
>>>>> OPTIONS.RESETALL,
>>>>> - OPTIONS.RESET,
>>>>> + OPTIONS.HEADLESS,
>>>>> OPTIONS.CHECK
>>>>> });
>>>>
>>>> why the change of order??
>>>
>>> RESET and RESETALL both check for -reset when parsing and the first one in
>>> the list is the option that gets recognized, meaning either I switch the
>>> order or I change the option I am checking value with, to be RESETALL. So
>>> only one is really parsed for which is RESET, but I left RESETALL since
>>> for the help message I think it is a nice entry to show that -reset all is
>>> a special property.
>>>
>>
>> oh interesting.... ok then.
>>>>> }
>>>>> @@ -210,7 +213,7 @@
>>>>> l.addAll(getJavaWsRuntimeOptions());
>>>>> l.addAll(getJavaWsControlOptions());
>>>>> //trustall is not returned by getJavaWsRuntimeOptions
>>>>> - //or getJavaWsControlOptions, as it is not desitred in
>>>>> documentation
>>>>> + //or getJavaWsControlOptions, as it is not desired in
>>>>> documentation
>>>>> l.add(OPTIONS.TRUSTALL);
>>>>> return l;
>>>>> }
>>>>
>>>> Any way, please push this part of this patch
>>>> = changes to netx/net/sourceforge/jnlp/OptionsDefinitions.java
>>>> + according properties IBOCheck, new one for
>>>> EVEN_NUMBER_OR_WITHEQUALCHAR. and BOHelp
>>>>
>>>> About the BOHelp, I'm for small rewording. What about
>>>> +BOHelp = Prints out information about supported command and basic
>>>> usage.
>>>>
>>>> ?
>>>
>>> I like it :) changed it.
>>>
>>>>
>>>> Do as your wish, and please push this specified part of your patch.
>>>
>>> I attached the patch you wanted me to push, since there is one small change
>>> in that since nodashhelp is removed I had to change CommandLine.java to
>>> check for OPTIONS.HELP instead of OPTIONS.NODASHHELP on one line.
>>> Here is the changelog I plan to add to it
>>>
>>> 2014-10-07 Lukasz Dracz <ldracz at redhat.com>
>>>
>>> Standardize all options to use hyphens
>>> * netx/net/sourceforge/jnlp/OptionsDefinitions.java:
>>> itweb-settings options changed to have hyphens in front,
>>> added new enum to NumberOfArguments
>>> (getItwsettingsCommands): added headless, changed nodashhelp to help
>>> * netx/net/sourceforge/jnlp/controlpanel/CommandLine.java
>>> * netx/net/sourceforge/jnlp/resources/Messages.properties:
>>> (BOHelp, IBOCheck): modified (NOAevennumberorequalschar): added
>>>
>>>>
>>
>> looks ok to me. One minor - tehr eis one method strongly violating formating
>> rules, please format
>> :) (if you dnt find it, its on the end of the email [1]
>>
>> after the formating of this method, ok to push.
>>
>>>> Will try at least something from the rest, but you are doing so much
>>>> things
>>>> in so much complicated
>>>> ways to achive so simple results:(( Maybe you can arrange face2face
>>>> meeting
>>>> with Jie and he mayhelp
>>>> you to establish basic structures more simple?
>>>>
>>>> But at least we agreed on "what should be done"
>>>> ...
>>>
>>> This reply is mainly to make sure that the changes to what you wanted to be
>>> pushed are approved.
>>> I agree my code has become messy and unnecessarily complicated, I will look
>>> into simplifying it
>>> as much as I can.
>>>
>>>> + for (String arg : args) {
>>>> + if (args.indexOf(arg) % 2 == 0) {
>>>>
>>>> Again this terrible modulo?
>>>>
>>>>
>>>> Why you dont just get list of the parameters, then iterates +2 and get(i)
>>>> and
>>>> get(i+1) ?
>>>> You are terribly leaking the parsers encapsulated functionalities. Also,
>>>> why
>>>>
>>>> + String key = "";
>>>> + String value;
>>>>
>>>> is declared out of the loop?
>>>>
>>>>
>>>> ugh, isNextOption? isCurrentOption? whyyyyyy? ouch whyyyy:(
>>>>
>>>> you you really should get only list<[key,list<string>] /list of objcets,
>>>> where is key + its
>>>> arguments in another list. This strucutre IS the result of parsing, and
>>>> parsing is exactly what you
>>>> parser is doing.
>>>>
>>>> and then iterate through it, no more complications around.
>>>>
>>>> (ps - the usage of isNextOption have its reasons in other palces of your
>>>> impls)
>>>>
>>>>
>>>> + if (args.contains("all")) {
>>>> resetAll = true;
>>>> + if (args.size() > 1) {
>>>> + for (String arg : args) {
>>>> + if (!arg.equals("all")) {
>>>> +
>>>> OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
>>>> R("CLUnknownCommand", arg));
>>>> + }
>>>> + }
>>>> + }
>>>>
>>>> Why this simple logic so complicatedly written?
>>>> for (int count = 0; count < optionParser.getNumberOfOptions(); count++)
>>>> +
>>>> optionParser.nextOption();
>>>>
>>>> is really terrible. If you wont to do so, then lets your parser implements
>>>> iterable, and do it
>>>> properly. Or iterate by index on the result of parsing, but do nt do this
>>>> terrible mixture.
>>>> But I think that that parser should be long ago splitted into two classes
>>>> - one - responsible for parsing, and second respnsible for varisous
>>>> operations above parsed items
>>>> (result of parsing).
>>>>
>>>> Feel free to do this refactoring as separate changeset before this actual
>>>> patch, or simply ignore me.
>>>>
>>>>
>>>>
>>>> try {
>>>> + optionParser = new OptionParser(args,
>>>> OptionsDefinitions.getItwsettingsCommands(), true);
>>>> + } catch (UnevenParameterException e) {
>>>> +
>>>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL,
>>>> e.getMessage());
>>>> + JNLPRuntime.exit(ERROR);
>>>> + }
>>>>
>>>>
>>>> for boot.java, keep the exception as debug only, for commandline, hhmhm
>>>> well
>>>> yes, erro_all should be ok.
>>>
>>> okay
>>>
>>>>
>>>> - public OptionParser(String[] args, List<OptionsDefinitions.OPTIONS>
>>>> options) {
>>>> + public OptionParser(String[] args, List<OptionsDefinitions.OPTIONS>
>>>> options, boolean
>>>> orderMatters) throws UnevenParameterException {
>>>>
>>>>
>>>> no No NO. No order metters here. PArser do not care. PArser do parsing.
>>>> And
>>>> prepare datat structure.
>>>> Other operation may depend o it, but then the parammeter hsould go to
>>>> them.
>>>>
>>>>
>>>>
>>>> + private void checkNumberOfArgumentsIsEven(boolean orderMatters)
>>>> throws
>>>> UnevenParameterException {
>>>> + String exceptionMessage;
>>>> +
>>>> + if (orderMatters) {
>>>> + exceptionMessage = checkEachOptionOccurenceHasEvenParams();
>>>> + } else {
>>>> + exceptionMessage = checkTotalOptionOccurenceHasEvenParams();
>>>> + }
>>>> +
>>>> + if (exceptionMessage != "") {
>>>> + throw new UnevenParameterException(exceptionMessage);
>>>> + }
>>>> + }
>>>> +
>>>> + private String checkEachOptionOccurenceHasEvenParams() {
>>>> + String exceptionMessage = "";
>>>> + for (ParsedOption parsed : parsedOptions) {
>>>> + if (parsed.getOption() != null &&
>>>> parsed.getOption().hasEvenNumberOrWithEqualsChar()) {
>>>> + if (parsed.getParams().size() % 2 != 0) {
>>>> + exceptionMessage += R("OPUnevenParams",
>>>> parsed.getOption().option);
>>>> + }
>>>> + }
>>>> + }
>>>> + return exceptionMessage;
>>>> + }
>>>> +
>>>> + private String checkTotalOptionOccurenceHasEvenParams() {
>>>> + Map<String, List<String>> evenNumbersTracker = new HashMap<>();
>>>> + String exceptionMessage = "";
>>>> + for (ParsedOption parsed : parsedOptions) {
>>>> + if (parsed.getOption().hasEvenNumberOrWithEqualsChar()) {
>>>> + if (evenNumbersTracker.isEmpty()) {
>>>> + evenNumbersTracker.put(parsed.getOption().option, new
>>>> ArrayList<String>());
>>>> + } else {
>>>> + for (String pop : evenNumbersTracker.keySet()) {
>>>> + if (pop != parsed.getOption().option) {
>>>> +
>>>> evenNumbersTracker.put(parsed.getOption().option,
>>>> new ArrayList<String>());
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> evenNumbersTracker.get(parsed.getOption().option).addAll(parsed.getParams());
>>>> + }
>>>> + }
>>>>
>>>>
>>>> I really did nto transalted what this hell code is doing and why it is
>>>> needed.
>>>>
>>>> From the new methods in oarser, imho only getAllValues have sens. Other
>>>> are
>>>> bringing in extremly
>>>> unclear code.
>>>>
>>>> imho you have only one possibility. To separate parser and parsed data
>>>> (you
>>>> already have
>>>> ParsedOption, and it is good). Whensome class wonts to do some iterations
>>>> abow parsed data, lets
>>>> give him the unmodificable llists for iterations. why not?
>>>>
>>>>
>>>> + public void removeParam(String param) {
>>>> + params.remove(param);
>>>> + }
>>>>
>>>>
>>>> why that? This itemshould be as immutable as possible.
>>>>
>>>>
>>>> I liek the exception handling.
>>>>
>>>>
>>>>
>>>>
>>>> Please - one note. you are adding to much overhead to really simple task.
>>>> Your ideas are good, but
>>>> the code design is really terrible. Each review costs me several hours. I
>>>> can
>>>> not afford it. Please
>>>> really try to sit withjie or withanybody in Torronto, and let him help to
>>>> redesign this patch.
>>>>
>>>>
>>>> Sorry for saying it:(
>>>
>>> No don't be ;)
>>> I'm sorry for costing you several hours :(
>>> I'm going to simplify it the best I can
>>> and see if Jie or someone else if they are not busy can quickly review it
>>> before sending it to the list again.
>>>
>>> Thanks for the review !
>>
>> One hint to my previosu review:
>>
>> Please split the patch to three parts
>> - first - add support for the list ParsedOption
>> - second - add support for the equls chars
>> - third - integrate it into itweb settings
>> - fourth - policy editor.
>>
>>
>> I think it will be much simpler to write, and even much simpelr to review.
>>
>>
>> J.
>>
>>
>>
>> [1] public boolean hasEvenNumberOrWithEqualsChar() { return
>> numberOfArguments ==
>> NumberOfArguments.EVEN_NUMBER_OR_WITHEQUALCHAR; } BLEEEEE
>>
More information about the distro-pkg-dev
mailing list