Fix #601 - better getopt compatibility for -- and --help handling

[rmunn]

This changes the tokenizer to use the same logic as glibc's getopt, where the handling of one token will influence the handling of the next token. In particular, anything that follows an option expecting a value, even if it is something specially handled, will become the value of that option. So if -s is an option expecting a string value, then myprog -s --help will NOT display the help screen, but will instead give -s the value "--help", with no -- needed.

To prove that this is how getopt behaves (and is expected to behave), run the following on any Linux machine: gzip -S --help README.md. The -S option lets you pick a suffix different from .gz, so if there's a README.md file in the current directory, this will create a gziped README.md--help file. This PR adjusts the behavior of CommandLineParser to mimic getopt's behavior exactly in this regard.

[rmunn]

Note that I wrote this PR on top of #594, because creating lists of input files with -i file1 -i file2 -i file3 is how getopt is supposed to work, and the current behavior of -i file1 file2 file3 is not getopt-compatible. I would eventually like to see -i file1 file2 file3 become an option that has to be explicitly enabled, and #594 become how CommandLineParser works by default.

This means that if you merge this PR as-is, #594 will automatically be merged as well.

[moh-hassan]

@rmunn
Thanks to your work.

This means that if you merge this PR as-is, #594 will automatically be merged as well.

and also #610 will automatically be merged as well.

[moh-hassan]

It is better to minimize the change in the public API to avoid break change ( except it is necessary), can we remove FlagCounter and it is considered as enabled when multiInstance is true, especially it's special case for multiInstance.

[rmunn]

FlagCounter is a necessary addition to enable the -vv scenario, otherwise since it's defined as an int property it would be assumed to expect a value, so -vv would try to parse the second v as an int and would fail. It's necessary to add a flag so that options that are expected to be flag-like but passed multiple times (e.g., -v for verbose) can be distinguished from options that are expected to be passed only once but take a value (e.g. -l 5 for "limit 5" in Git logs). Setting FlagCounter to be considered enabled when multiInstance is true would cause the -l 5 scenario to fail, as there would then be an ambiguity between "assign 5 to the -l option" and "assign 1 to the -l option since it's only specified once, and then assign 5 to the next Value in line".

I'll add tests for FlagCounter. The tests I just pushed are to test the scenarios that came up in discussion here: #610 (comment)

[rmunn]

I'm going to remove the FlagCounter feature from this PR: it's proving more difficult to implement than I had anticipated, and is going to require some fundamental reshuffling of how flag options are handled (a FlagCounter option needs to be treated more like a sequence than a flag throughout most of the code, up until the point where values are assigned). The required code changes to implement FlagCounter are making this PR drift out of its scope, so I'm going to pull them out of this PR and submit a separate PR for the FlagCounter feature once I get it working.

[rmunn]

Update: turns out I was almost there with my FlagCounter implementation yesterday, and I just needed a one-line change to get it all working. So I'm reverting my reversion of the feature, backing commit b8b9a97 out of this PR, and replacing it with commit 1e791b5 that implements FlagCounter properly and has a unit test to verify that it's working.

@moh-hassan - With this commit, I believe I've addressed all the things you mentioned in your code review and it's ready to merge if you're satisfied. With the new FlagCounter implementation, though, you might want to do another code review. If so, I'll be happy to address any further changes you might want me to make.

[moh-hassan]

@rmunn
I try to finalize both #594 and #610 then merging to develop branch because your PR depends on them.

I'm still in the code review for #607.
Kindly can you reply on my comment especially note 2, 3 and 4 because there is a major change in Tokenizer and may cause a break change and moving to V3.

[rmunn]

@moh-hassan - I had missed that comment. I'm reading it now and will respond to it over there.

This PR includes #594 so if you merge this one, #594 will automatically be merged as well. This one also contains a rewrite of #610 (with credit for the relevant commit still given to @robnasby), so merging #610 would cause some conflicts when you merge this one. If you decide that both #610 and this PR are worth taking, I'd suggest NOT merging #610 as-is, since this one replaces it. That way you'll have fewer merge conflicts to deal with.

[moh-hassan]

It's a best practice that every PR is one feature at a time per branch. This simplify maintenance and review, even if there is a minor confliction.
#594 and #610 have no breaking change, but #607 have a major breaking change and still need more discussion as I pointed in note 2,3 and 4.
Kindly, read my comments and comment for note 2,3 and 4.
I take your consideration of #610 and delay approving it.

[rmunn]

I've responded to notes 2, 3 and 4 in the other thread. I'll summarize my comments here:

Note 2, about AutoHelp=false: I believe this PR honors AutoHelp=false. If it doesn't, that's a bug and I'll fix it.

Note 3, about -- being allowed to be used as a value to another option: that should be the default, without any need for a ParserSettings change, because that's how getopt behaves. The fact that CLP currently doesn't allow this is precisely the bug I reported in #601 and that I am fixing here.

Note 4, about allowing an "option" (by which I think you mean a string value starting with -, whether or not it's currently a valid option specified by the user) to be used as a value to another option: again, that should be the default, without any need for a ParserSettings change, because that's how getopt behaves. The fact that CLP doesn't allow this is a bug, which I reported in #601 and which I'm fixing here.

BTW, about this being a breaking change: I actually was expecting this to be a breaking change, since I had a big rewrite of the tokenizer. But I was amazed to discover that after I rewrote the tokenizer, all the unit tests still passed! So this isn't a breaking change, just a bugfix. It turns out that after this fix, the behavior of CommandLineParser is just as it was before, except that a couple of bugs (-- not being allowed as a value, for example) are now fixed. And I would be really surprised to find that anyone was actually relying on the buggy behavior, because (as you're probably tired of hearing me say by now), the old behavior was not how getopt works, and anyone using this library is probably expecting getopt-like behavior.

So although I won't object if you decide that this PR needs a new major version number once it goes in, I do think that a major version number bump is not necessary here, because it's a bugfix that brings the behavior more in line with what it's intended to be. It's also not at all a breaking change from the point of view of the API, because the user-facing API has no changes except for a new OptionAttribute property (FlagCounter) being available.

Having said that, I do think that more documentation is needed, and once I know you're planning to accept this PR I'll start working on editing the wiki to explain precisely how getopt, and now CLP, handles option values. Because I do think that the documentation is lacking in how it explains getopt's corner cases which CLP mimics, and it might be good to make sure they know what to expect.

[moh-hassan]

I'll jump temporary for using #607, for running sample programs already using v2.8.
I run the program using v2.8 and the Pr607 and compare the result:

Case#1:
I use custom parser with EnableDashDash = true;
I pass plugin options PluginArgs to a third party program:

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

commandline: I pass -- so parser can consider all options after -- as a value.

$ myprog -n abc -p -- -x -y -z 44"

In v2.8
I catch all arguments -x -y -z 44 correctly in PluginArgs

in PR607, i get an error

DemoClp 1.0.0
Copyright (C) 2020 DemoClp

ERROR(S):
  Option 'x' is unknown.
  Option 'y' is unknown.
  Option 'z' is unknown.

  -n

  -p

  --help       Display this help screen.

  --version    Display version information.

I want to migrate the program to PR607 with zero change in the source code to avoid breaking change (if possible).
So, what is the right way to migrate myapp running in v2.8 to work properly in PR607?

[rmunn]

That was always an incorrect way to use --, and I for one would be filing a bug report if I discovered that as a user I was expected to specify the PluginArgs that way, but I can see your point that some people might be relying on that behavior, so I guess it should be a breaking change after all.

The right way to use a PluginArgs option like that would be to do one of two things:

1. Specify a separator like ' ' (space) and pass the PluginArgs as -p "-x -y -z 44" (which was not possible before Fix #601 - better getopt compatibility for -- and --help handling #607, but is the right way to do it), or

2. Turn on AllowMultiInstance, which then lets the user pass the PluginArgs one at a time such as -p -x -p -y -p -z -p 44; again, this would not have been possible before Fix #601 - better getopt compatibility for -- and --help handling #607 (Add multi-instance option support #594 adds AllowMultiInstance, but Fix #601 - better getopt compatibility for -- and --help handling #607 allows values that start with - to be normal option values without having to misuse -- for that purpose).

Both do require a small modification to the user's code, so I guess we do have to consider this a breaking change even though it's really a bugfix.

[rmunn]

Replaced by #684, which adds a GetoptMode option to the parser settings so that people have to specifically opt-in to this behavior. Closing this PR in favor of #684.