[rfc][icedtea-web][itweb-settings] itweb-settings use Option Parser
Lukasz Dracz
ldracz at redhat.com
Tue Oct 21 18:26:24 UTC 2014
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
> >>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: parsedOption-2.patch
Type: text/x-patch
Size: 38646 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141021/c815f4ff/parsedOption-2-0001.patch>
More information about the distro-pkg-dev
mailing list