[rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser
Jiri Vanek
jvanek at redhat.com
Tue Nov 4 13:13:34 UTC 2014
Hi!
This looks ok.
With some fear.. go on and push :)
J.
On 10/21/2014 08:26 PM, Lukasz Dracz wrote:
> Hello,
>
>> From: "Jiri Vanek" <jvanek at redhat.com>
>> To: "Lukasz Dracz" <ldracz at redhat.com>
>> Cc: "IcedTea" <distro-pkg-dev at openjdk.java.net>, "Jie Kang" <jkang at redhat.com>
>> Sent: Tuesday, October 21, 2014 10:45:23 AM
>> Subject: Re: [rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser
>>
>> 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
>
> Okay added back the method removeLeadingHyphens, to support this 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.
>
> I decided to keep it in this patch. Also renamed EVEN_NUMBER_OR_WITHEQUALCHAR to EVEN_NUMBER_SUPPORTS_EQUALS_CHAR, I think this might be a better name.
>
>> - 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
>
> Removed it.
>
>>
>> More in IRC log :)
>>
>> Thank you both!
>
> Thank you,
> Lukasz Dracz
>
>> 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