Properly assign arguments after a double dash to values

[robnasby]

This is a working solution to issue #605, which properly assigns arguments after a double dash to values, rather than options. However, I'm not particularly happy with the solution, so I'm looking for feedback.

I created a new token type ValueForced, which is only used by the Tokenizer when EnableDashDash is set and -- is encountered. Sequence will not select ValueForced tokens. I think the overall pattern is fine, but I don't like the name ValueForced for the token. Unfortunately, the work Value is already overloaded quite a bit, which is why I struggled with a better name.

I also want to add a test to verify that parsing fails if there are arguments following the double dash, but the options object doesn't include any Value properties. If there are any other tests you'd like me to add, let me know. Thanks!

[rmunn]

This will almost certainly conflict with #607, where I rewrote the guts of the Tokenizer to allow for proper getopt compatibility (see #601 for rationale). I'm a little short on time this week, but I'll see if I can incorporate your changes, or something equivalent, into my PR branch for #607 so that -- becomes a true barrier to multi-valued options.

[robnasby]

Sounds good @rmunn. I'm also happy to rebase onto #607 when it is merged, if that's quicker / easier.

[rmunn]

I gave the rebase a try today and discovered that it will take a complete rewrite, which is more than I have time for at the moment (I only have little bits of time for this project this month). The tokenizer isn't the big issue; the big issue is Sequence.cs, which was completely changed by #594 (which is a PR that I created, but the actual code is by @tydunkel and I don't fully understand it yet). All my other PRs (#607 and #608) I wrote on top of #594, and since I don't yet understand the changes to Sequence.cs that @tydunkel made for #594, I won't be able to do the rebase until I have time to properly sit down and understand the code, which won't be for a few more weeks given my other obligations.

So @robnasby, if you want to rebase this on top of #608 (probably better than rebasing on top of #607), you'll need to check out the feature/autohelp-rewrite branch of https://github.com/rmunn/commandline/ and then rewrite your code on top of the Sequence.cs implementation found there.

[rmunn]

P.S. When I rewrote the guts of the tokenizer in #607 I ended up making the ExplicitlyAssigned property meaningless, so if you want to remove it (or rename it to ValueForced), that might let you implement your logic for this PR with fewer changes to the Token API.

[rmunn]

@robnasby - I've modified your PR and incorporated it into #607 in b6d3d47. Want to take a look and see if it looks right to you?

[moh-hassan]

@robnasby, @rmunn
Thanks for your work. The PR solves one of a waiting feature.

[moh-hassan]

@robnasby , @rmunn
It's nice to fire an error message when extra values are not consumed, example when IgnoreUnknownOptions=false (the default)

myprog option1,option2 option3 -- value1  value2  value3 value4

with error message like: values [value3, value4] are extra values

 class Options
        {
             [Option('o', "option", Separator = ',')]
            public IEnumerable<string> Option { get; set; }
            [Value(1)]
            public string Value2 { get; set; }
           
            [Value(0)]
            public string Value1 { get; set; }      
        }

[moh-hassan]

Please, add xml documentation for all new properties/ .. with explanation using comments.

[moh-hassan]

This test Fail.

 class CommandLineOptions3
        {      
            [Value(0)]
            public IEnumerable<string> Option { get; set; }  
            [Value(1)]
            public string  Value1 { get; set; }
            [Value(2)]
            public string Value2 { get; set; }
        }

Result:

commandline: one two three -- --option3 value4
options:
{
  "Option": [
    "one",
    "two",
    "three",
    "--option3",
    "value4"
  ],
  "Value1": null,
  "Value2": null
}

Expected:

options:
{
  "Option": [
    "one",
    "two",
    "three"   
  ],
  "Value1": --option3,
  "Value2": value4
}

[rmunn]

@moh-hassan - As I understand the docs in https://github.com/commandlineparser/commandline/wiki/Getting-Started#value-attribute, if an IEnumerable<string> property has the attribute Value(N) and does not define a Max for that IEnumerable, then any Value(N+1), Value(N+2), etc. properties will always be blank, because the IEnumerable<string> property is supposed to consume all values. Quote from the docs: "There's no point defining a Value attribute with a higher index than that of a sequence Value which lacks a Max range constraint."

The documentation says nothing about how -- should interact with that feature, but I would expect it to change nothing about how values are handled, since -- means "stop processing options and consider everything after this point to be a value". I.e., given this definition:

 class CommandLineOptions
        {      
            [Value(0)]
            public string Value0 { get; set; }  
            [Value(1)]
            public string  Value1 { get; set; }
            [Value(2)]
            public string Value2 { get; set; }
        }

I would expect all four of the following command lines to put a in Value0, b in Value1, and c in Value2:

myprog.exe a b c
myprog.exe -- a b c
myprog.exe a -- b c
myprog.exe a b -- c
myprog.exe a b c --

And given what the docs say, in the options class you defined:

 class CommandLineOptions3
        {      
            [Value(0)]
            public IEnumerable<string> Option { get; set; }  
            [Value(1)]
            public string  Value1 { get; set; }
            [Value(2)]
            public string Value2 { get; set; }
        }

I would expect all of the following command lines to behave exactly the same as well, putting { "a", "b", "c" } into Option (which really should be named Value0 as its attribute is not an Option, it's a Value) and putting empty strings into Value1 and Value2:

myprog.exe a b c
myprog.exe -- a b c
myprog.exe a -- b c
myprog.exe a b -- c
myprog.exe a b c --

All five of those lines should produce exactly the same result, given that we are not dealing with Options, but Values.

Here's another example. Consider a shell script that does something like this:

#!/bin/bash

myprog.exe -- $(ls $SOMEDIR/*.txt)

Here the intent is to process all text files in the directory stored in the SOMEDIR variable. (The $() syntax, for anyone more familiar with Windows than with Unix, is processed by the Bash shell before it passes arguments to the program, and it takes the output of the program and turns it into text that will be passed as arguments.) The options for myprog.exe contain a bunch of Options for telling the program how to process the input files, and a single Value of type IEnumerable<string> to capture the input filenames. The user is quite sensibly putting a -- before the list of filenames Then the user says "Oh, there's also another file I want to handle". And he adds it as the first argument, like so:

#!/bin/bash

myprog.exe somefile.txt -- $(ls $SOMEDIR/*.txt)

Speaking as an experienced Linux user, I would be quite surprised, and would file a bug report, if that command line processed only somefile.txt and didn't process the contents of $SOMEDIR. Because in a Linux command line program, myprog.exe somefile.txt -- $(ls $SOMEDIR/*.txt) and myprog.exe -- somefile.txt $(ls $SOMEDIR/*.txt) should be handled identically. The CommandLineOptions3 test you wrote in #610 (comment) would not handle those two command lines identically, so it's an incorrect test.

[rmunn]

@robnasby , @rmunn
It's nice to fire an error message when extra values are not consumed, example when IgnoreUnknownOptions=false (the default)

myprog option1,option2 option3 -- value1  value2  value3 value4

with error message like: values [value3, value4] are extra values

That would be a behavior change from the current code, which does not warn (or throw an error) about extra values at the end of the command line. I checked by running the test you suggested here, against the current state of both the master and develop branches. And in both master and develop, with IgnoreUnknownOptions = false, CommandLineParser currently silently ignores any extra "value" parameters and does not throw an error or warn about them.

I don't mind changing that behavior, but I think that change belongs in a different PR and would be out of scope for this one.

[moh-hassan]

@rmunn

myprog.exe somefile.txt -- $(ls $SOMEDIR/.txt) and myprog.exe -- somefile.txt $(ls SOMEDIR/.txt) should be handled identically

You are correct,

-- is used as you said "stop processing options and consider everything after this point to be a value"
or in other words "after this point, consider any option starting with - or -- as a value"

In pr #610, it's intended to use -- with extra role as a "value separator between IEnumerable<string> option and the next values, and that is ok,

The CommandLineOptions3 test you wrote in #610 (comment) would not handle those two command lines identically, so it's an incorrect test.

The test case that I provided is intended to use -- as a value separator between IEnumerable<string> ValueAttribute and it is expected that the PR consume all values up to -- (not forced) and assign them to the IEnumerable property.

If the test case is incorrect (i.e not applicable or not supported) we can add this in Wiki and provide guidelines for developers how to use this feature effectively.

But really, it can be handled and -- do its extra role as a value separator.
That needs other change in the code like ValueMapper, but we can handle it in the future with other PR.

don't mind changing that behavior, but I think that change belongs in a different PR and would be out of scope for this one.

Ok, it can be in a different PR.

So I question whether it's a good idea to add XML docs here when the original author(s) chose to leave them out.

Xml doc help developer and reviewer to understand the flow of program and help in future maintenance, even if it's missed in the original author.

[rmunn]

The way -- is treated in getopt is that any text after -- is considered a value, even if it starts with a hyphen. But since getopt doesn't allow options with multiple values the way CLP does them (in getopt, each option that takes a value must be followed by exactly one value), we have to guess what the semantics would be. However, getopt does allow options to appear multiple times with a single value each time, like #594 allows for, so we can get a clue from the way the semantics would work in that case.

If your getopt-using program had a-i option for specifying multiple input files, myprog -i file1 -i file2 would cause getopt to process the -i option twice; since your program expects it to sometimes occur more than once, you would have a string list allocated for your -i values, and every time getopt reported a -i value to you, you'd append it to your list of strings. So what would happen if there was a -- in there (NOT as a -i value)? I.e., myprog -i file1 -i file2 -- -i file3? What would happen is that the third -i would not be processed by getopt, and you'd end up with a -i value of { "file1", "file2" }, and the remaining two arguments ("Values" in CLP terminology) would be "-i" and "file3".

But what would happen in getopt if you didn't use -i for specifying your input files, but just used non-option arguments ("Values" in CLP) for specifying them? Well, myprog file1 file2 would, of course, end up with "file1" and "file2" in your extra-args array. And if there was a -- in there, like myprog file1 file2 -- file3, then your extra-args array would end up containing "file1", "file2", and "file3". The -- would be removed, and all three of the extra would end up in the same array. Which is different behavior than the way it would be if you had used an option like -i, where the -- would end up causing getopt to stop processing option values.

So it makes sense to me that in CLP, a -- would break up an IEnumerable Option but would NOT break up an IEnumerable Value — because that's the closest simulation of getopt's behavior that we can reach, given the extra feature we have of allowing options to take multiple values.

In other words:

The test case that I provided is intended to use -- as a value separator between IEnumerable ValueAttribute and it is expected that the PR consume all values up to -- (not forced) and assign them to the IEnumerable property.

If the test case is incorrect (i.e not applicable or not supported) we can add this in Wiki and provide guidelines for developers how to use this feature effectively.

I believe that this test case is not applicable, because it's testing for behavior that CLP should not have. The behavior I believe CLP should have, since it tries to mimic getopt as closely as possible, is that -- should only break up enumerable OptionAttributes and should NOT break up any enumerable ValueAttributes.

[moh-hassan]

This test case fail but success in v2.8:

 [Fact]
        public void Enable_dash_dash_true_for_IEnumerable_test()
        {
            //Arrange
            var args = "-n abc -p -- -x -y -z 44".Split();
            //Act
            var parser = new Parser(with =>
              {
                  with.EnableDashDash = true;
              });
            parser.ParseArguments<Options610>(args)
                .WithParsed(opt =>
                {
                    var expected = new[] { "-x", "-y", "-z", "44" };                   
                    opt.PluginArgs.Any().Should().BeTrue();
                    opt.PluginArgs.Should().BeEquivalentTo(expected);
                });

        }

  class Options610
        {
            // plugin name
            [Option('n')]
            public string Name { get; set; }
            //plugin options 
            [Option('p')]
            public IEnumerable<string> PluginArgs { get; set; }

        }

[rmunn]

@moh-hassan That test case is testing for the very bug that #605 is all about, and that this PR is trying to fix. @robnasby (and I as well) believe that anything after -- should not be treated as the contents of an IEnumerable Option. What both he and I expect CLP to do with -n abc -p -- -x -y -z 44 is for the -p to be an empty IEnumerable, and for there to be four Values, -x, -y, -z, and 44.

So to write that test case properly, so that it fails when the bug is present but succeeds once the bug is fixed, you should reverse what it expects and write it like this:

 [Fact]
        public void Enable_dash_dash_true_for_IEnumerable_test()
        {
            //Arrange
            var args = "-n abc -p -- -x -y -z 44".Split();
            //Act
            var parser = new Parser(with =>
              {
                  with.EnableDashDash = true;
              });
            parser.ParseArguments<Options610>(args)
                .WithParsed(opt =>
                {
                    var expected = new[] { "-x", "-y", "-z", "44" };                   
                    opt.PluginArgs.Any().Should().BeFalse();
                    opt.ExtraArgs.Any().Should().BeTrue();
                    opt.ExtraArgs.Should().BeEquivalentTo(expected);
                });
        }

  class Options610
        {
            // plugin name
            [Option('n')]
            public string Name { get; set; }
            //plugin options 
            [Option('p')]
            public IEnumerable<string> PluginArgs { get; set; }
            [Value]
            public IEnumerable<string> ExtraArgs { get; set; }
        }

[moh-hassan]

@rmunn

That test case is testing for the very bug ..

What is the bug here?

[rmunn]

The bug here is #605. The -- is supposed to separate Options from Values, and the user expectation as seen in #605 is that anything after -- would not become part of an Option. In other words, this is what the user expected:

    class CommandLineOptions
    {
        #region Properties

        [Option('o', "option")]
        public IEnumerable<string> Option { get; set; }

        [Value(0)]
        public IEnumerable<string> Values { get; set; }

        #endregion
    }

Running the program as follows:

foo.exe -o "option" -- value1 value2 value3

was expected to produce the results:

options.Option = [ "option" ]
options.Values = [ "value1", "value2", "value3" ]

Because the -- separates options from values, and therefore @robnasby expected that anything after the -- would not be assigned to an option.

[moh-hassan]

@robnasby
Please, can you rebase to develop and resolve confliction, so I can merge your PR.
Note that PR 594 is merged into develop.

[moh-hassan]

I have resolved the confliction and merged the PR to develop branch for release v2.9.0-preview2

[moh-hassan]

Merged to develop manually

[icedoor]

@moh-hassan Will there be a new release including this fix any time soon? It was committed on Aug 5, 2020 and there has not been a release since Jul 24, 2020.
Thanks!

[moh-hassan]

Hi @icedoor

Will there be a new release including this fix any time soon? It was committed on Aug 5, 2020 and there has not been a release since Jul 24, 2020.

I can't publish a new release because the nuget key expired and waiting the project owner to renew the key.

[icedoor]

Hi @moh-hassan ,

Thanks for the info!
Can we escalate this to the project owner to get a new nuget key to be able to make a new release asap? I think a lot of people are depending on this use case to work as expected.

Thanks

[moh-hassan]

Can we escalate this to the project owner to get a new nuget key to be able to make a new release asap? I think a lot of people are depending on this use case to work as expected.

CC/ @ericnewton76