Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multi-instance option support #594

Merged

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Mar 12, 2020

#357 is a request for supporting multiple occurrences of the same parameter and collecting those into an enumerable, e.g. "myprog -i file1 -i file2 -i file3" would parse as having the -i parameter with three values, file1, file2 and file3. @tydunkel created an implementation of this feature, but never (AFAICT) created a PR for it. This PR is @tydunkel's code, rebased onto the current develop branch.

Note that when I run this, I get some spurious unit test failures and some real failures. The spurious failures I'm getting are caused by #403, but the real failures I'm getting all have the same pattern: the "ERROR(S)" section of the output is repeated twice. E.g., this test expects the parser to output:

testhost 15.3.0
Copyright (C) 2020 author
ERROR(S):
No verb selected.
add        Add file contents to the index.
commit     Record changes to the repository.
clone      Clone a repository into a new directory.
help       Display more information on a specific command.
version    Display version information.

and instead it's outputting:

testhost 15.3.0
Copyright (C) 2020 author
ERROR(S):
No verb selected.
ERROR(S):
No verb selected.
add        Add file contents to the index.
commit     Record changes to the repository.
clone      Clone a repository into a new directory.
help       Display more information on a specific command.
version    Display version information.

I don't have time right now to track down why the error output is being duplicated. But all the unit test failures I could see look like they're the same thing, so once that single bug is located and fixed, the tests should all pass.

Hopefully this will be helpful to someone to be able to move forward with implementing #357 (and #522 which is a dupe of #357).

@rmunn rmunn force-pushed the feature/multi-instance-args branch from d846fb5 to 78171b0 Compare March 13, 2020 09:24
@rmunn
Copy link
Contributor Author

rmunn commented Mar 13, 2020

Now that #597 has been merged into develop, this PR (rebased onto current develop) now passes all unit tests on my development laptop, so I've pushed the rebased commit (78171b0) to this PR.

@EraYaN
Copy link

EraYaN commented Mar 17, 2020

This also helps with a command line like:

compress --include *.txt --include *.dat ./directoryforcompression

versus

compress --include *.txt *.dat ./directoryforcompression

with options like

class CompressOptions
{
    [Option('i', "include", Required = false, HelpText = "Include patterns.")]
    public IEnumerable<string> IncludePatterns { get; set; }
    [Value(0, MetaName = "BaseDir", Required = true, HelpText = "Directory to be compressed.")]
    public string BaseDir { get; set; }
}

The second one breaks since it will not assign the last value to the BaseDir option.

@robnasby
Copy link

@ericnewton76, any chance this could be included in v2.8.0?

@mylemans
Copy link

mylemans commented May 6, 2020

I'm waiting for this as well.

@mylemans
Copy link

Also wouldn't it be better to add this as a per-option option? Or allow both methods, where the option added by the PR work on a global scale.

@moh-hassan
Copy link
Collaborator

@tydunkel, @rmunn
Thanks for your work. That is a very needed feature.

@moh-hassan moh-hassan added this to the 2.9 milestone May 30, 2020
@moh-hassan moh-hassan merged commit ccd02e4 into commandlineparser:develop Jun 15, 2020
@moh-hassan
Copy link
Collaborator

moh-hassan commented Aug 2, 2020

@rmunn
V2.9.0-preview1 is published with this PR merged.
Using this version, this test case failed because there is a similar values. It works fine in v2.8.
Please, can you fix this bug to publish v 2.9.
PR 610 is merged and depends on this PR.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 14, 2020

@moh-hassan I have some time to work on this now, so I'll look into that test failure and let you know what I find.

@moh-hassan
Copy link
Collaborator

Thanks @rmunn for response.
JFI, Because the dependency between PR 610 and Pr 594, i had to revert both and then re-merged PR #610.
So, Please start rebasing PR 594 to develop branch.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 14, 2020

@moh-hassan Done. I opened a new PR since I don't seem to be able to re-open this one after GitHub thinks it was already merged. #678 is this PR, rebased onto the current state of the develop branch, and with bug you mentioned in #594 (comment) fixed (and a new unit test added to the code to verify that bugfix).

@rmunn
Copy link
Contributor Author

rmunn commented Aug 14, 2020

@moh-hassan BTW, my next plan is to rewrite #607 so that it is controlled by a Parser option. I'm thinking that I'll call the option GetoptMode. It will be off by default so that the current behavior of CommandLineParser is unchanged, but turning it on will enable strict getopt compatibility (in getopt's default mode, i.e. as if POSIXLY_CORRECT is turned off in getopt). In particular, turning on GetoptMode will cause -s -- -x (where -s is a string option and -x is a switch) to be interpreted as "-s gets the value --, and the -x switch is turned on", because of the specific way in which getopt handles option values. (As mentioned in our discussion in #601).

Do you plan to do a 2.9 release right away and I should expect to get GetoptMode into 2.10 but not 2.9? Or is the 2.9 release still at least 3-4 days off so I'll have time to get GetoptMode implemented and PR'ed?

@moh-hassan
Copy link
Collaborator

@rmunn Thanks for the new PR 678.

is the 2.9 release still at least 3-4 days off so I'll have time to get GetoptMode implemented and PR'ed?

Yes v2.9 can be delayed for the end of the week.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 19, 2020

@moh-hassan - GetoptMode PR is done: #684. Note that #682 should be merged first before #684. Once #682 is merged, I'll rebase #684 on top of the new state of the develop branch, and then #684's two failing unit tests will start passing. (They exercise the same bug that #682 fixes). My next PR will be to rewrite #608 so that it's rebased on top of develop, and I expect to have that done by this time tomorrow.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 19, 2020

@moh-hassan - And I've finished rebasing #608 on top of develop as well. Took me less time than expected because I had anticipated merge conflicts that didn't turn up. New version of #608 is #686.

@JacekArdanowski
Copy link

@rmunn could you tell why #594 has been dropped in 2.9.0-preview2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants