Introduce option groups

[hadzhiyski]

I found a missing use case in the commandline library.
When one or more options has group set, at least one of these properties should have set value (they behave as required)

Example:

[Option("option1", Required = true)]
public string Option1 { get; set; }

[Option("option2", Group="special")]
public string Option2 { get; set; }

[Option("option3", Group="special")]
public string Option3 { get; set; }

myapp --option1="test" --option2="asd" --option3="qwerty" // OK
myapp --option1="test" --option2="asd" // OK
myapp --option1="test" --option3="qwerty" // OK
myapp --option1="test" // Error: MissingGroupOptionError 
Error text: "At least one option in group 'special' must have value"

I hope this makes sense.

Closes #546

[hadzhiyski]

Could you review this and approve this PR if you think it is worth introducing the feature? For me it is useful, because I need it for a project of mine - https://github.com/hadzhiyski/FileWarden/tree/develop

[moh-hassan]

Thanks @hadzhiyski for your work.

As I understand group (correct me):

• group is like Mutual Exclusive Options except in group at least one option in group is required(they need not to be defined as required).

• you can use more than one option in the same group (even all options ) at a time.

The following use case using enum may be equivalent for the group:

class Options
{
[Option("option1", Required = true)]
public string Option1 { get; set; }

//use multi enum
[Option("special", Required = true)]
public IEnumerable<Special> SpecialOption { get; set; }
}

//stndardize option values using enum
enum Special 
{
asd,
qwerty
}

Mapping you test cases ,

This is valid command:

 myapp --option1 test --special asd  //ok only one special option
 myapp --option1 test --special qwerty  //ok only another one special option
 myapp --option1 test --special asd qwerty //ok both special options

This is not valid:

 myapp --option1 test  //Error  no Special option is provided (required)

Can this approach help for your use case?

[hadzhiyski]

Hello,

This is a useful solution. However it's not exactly what I am trying to achieve.
Imagine an application which appends prefix and/or suffix to a filename. Basically renames the file with prefix and/or suffix. Both options ("prefix" and "suffix") belong to the verb rename, so I am trying to achieve the following

myapp rename --source C:\test\file1.txt --prefix test --suffix _10122019 // OK. We have both options
myapp rename --source C:\test\file1.txt --prefix test // OK. We have prefix option only
myapp rename --source C:\test\file1.txt --suffix _10122019 // OK. We have suffix option only

But

myapp rename --source C:\test\file1.txt // Error: Neither prefix nor suffix is available

Mutually exclusive sets allow me use only one of the two options, but never together.
Your solution might be useful if I want to pass multiple values to either to the prefix or suffix option.

How does this sound to you? I hope it makes sense.

[moh-hassan]

I give it a quick run test.It seems it's useful and good.
Kindly, can you cover the following points:

• Provide unit test that cover the different use-cases including exception and helpText.
• What about more than one group? It seems it support more than one (good).
• A wiki page to explain the value of the group with code samples?
• Is there a break change? It seems that there is a break change in some tests?
• Can this feature help the concept of SubVerbs as explained here (just for information, need not change in your PR)

[hadzhiyski]

@moh-hassan How I could update the wiki? I cloned it, but when I try to push the changes, I am getting 'access denied' error. Could you add me to commandlineparser organisation or I can send you the changes here and you to update these for me later?

[moh-hassan]

You can follow the instructions of How to contribute to Wiki Documentation and attach the git patch file in PR.

[hadzhiyski]

Hello,
Here are the wiki updates. option-groups.zip

[hadzhiyski]

• Provide unit test that cover the different use-cases including exception and helpText.
• What about more than one group? It seems it support more than one (good).

We can use a single group on a single option which basically acts like required. Multiple groups can be used on multiple options and they are separated from each other.

• A wiki page to explain the value of the group with code samples?
• Is there a break change? It seems that there is a break change in some tests?

Group parameter is added to OptionSpecification and OptionAttribute

• Can this feature help the concept of SubVerbs as explained here (just for information, need not change in your PR)

In my opinion there is no connection between the two functionalities. Option groups are on 'option' level and sub verbs are on 'verb' level.

[moh-hassan]

Thanks @hadzhiyski for the update.
The wiki patch is applied successfully :) and it seems good.

There are some missing unit tests:

1. Required

Required rules are ignored if an option belongs to an option group.

Need a unit test

2. Default Value
   a-Need a unit test
   b- It's failed and should success if no values is provided for the group

   At least one option from group 'append' (prefix, suffix) is required.

Group should take Default into account.
(if (option ==null) option = Default)

3. What do you think for the priority of SetName /Group, which one can be ignored?
   [ ] ignore SetName in favor of Group.
   [ ] ignore Group in favor of SetName.
   [ ] Raise Error: Only SetName or Group can be used. The two are not allowed. (Preferred)

4. What is the effect of option with group if it's hide? (ignore all hiding options)

Edit.

[hadzhiyski]

Group should take Default into account.
(if (option ==null) option = Default)

@moh-hassan Could you give me more details about this? I can not understand what are you trying to say. Could you send a code sample or where this is located in source?

3. What do you think for the priority of SetName /Group, which one can be ignored?
Raise Error: Only SetName or Group can be used. The two are not allowed.

I also think the best way will be to raise error.

[moh-hassan]

@hadzhiyski

Group should take Default into account.
(if (option ==null) option = Default)

It means that when using Group for option and that Option has Default like:

[Option("option2", Group="special", Default="xyz")]
public string Option2 { get; set; }

Group should take the Default value "xyz" into account.
I suggest you a resolution for this issue using a Pseudo code rule (not included in your source and like c# code)

//Enforce this rule for the group
if (option.Value ==null) option.Value = Option.Default)

You are free to implement this rule.
I think you may be confused with Default because you are using this class name: Simple_Options_With_OptionGroup_WithDefaultValue which isn't related to Default :)

[moh-hassan]

@hadzhiyski
Kindly, Stop changing your PR because I start reviewing yours.
I will ,a little time, send you the comments of review with the suggested changes.

[moh-hassan]

I suggest change the type of Group from Maybe to string, for the following reasons:

• Group is definded in OptionAttribute Class as string and here

• It's like string SetName which is initialized to string.Empty

• There is no null processing /or Mapping in the Group.It's just only a name.

• You need not to do such expressions:

      string.IsNullOrWhiteSpace(attribute.Group) ? Maybe.Nothing<string>() : Maybe.Just(attribute.Group),
  

I did these modification in my side and test pass except 3 tests in the multi group(not yet reviewed).

[moh-hassan]

All Test passed including the 3 multi group help test.

If you want, I can update your PR with a commit: "Modify type of Group to string instead of Mayb<string>"

[hadzhiyski]

@moh-hassan Thanks for the review.
If you already have the commit, please do it. Otherwise I will make the changes.

[moh-hassan]

PR is updated by the commit: Modify type of Group to string instead of `Maybe'

[hadzhiyski]

Brilliant. Thank you so much!

[hadzhiyski]

@moh-hassan Thank you for the great work!
In which version I could expect this feature to be included?

[sm-g]

@moh-hassan cli is confusing now

for such options:

internal class Options
{
	[Option('u', "url", SetName = "url", Group = "input", Default = "http://localhost:5002/swagger/current/swagger.json")]
	public string SwaggerUrl { get; set; }

	[Option('j', "json", SetName = "json", Group = "input")]
	public string SwaggerJson { get; set; }

	[Option('f', "file", SetName = "file", Group = "input")]
	public string SwaggerFilePath { get; set; }
}

when running exe without any args we have output:

 At least one option from group 'input' (u, url, j, json, f, file) is required.

  -u, --url       (Group: input) (Default: http://localhost:5002/swagger/current/swagger.json)

  -j, --json      (Group: input)

  -f, --file      (Group: input)

So, default value for url will be set only when exe invoked with exe -j "value" or exe -f "value". Which is not what I expect as user of program when see help message.

Also new Group feature does not prevent setting default value for "mutually exclusive" properties, it that by design?

[moh-hassan]

@sm-g
Both SetName and group are not allowed for the option in the same time. Use only SetName OR Group.

Removing SetName

internal class Options
{
	[Option('u', "url",  Group = "input", Default = "http://localhost:5002/swagger/current/swagger.json")]
	public string SwaggerUrl { get; set; }

	[Option('j', "json",  Group = "input")]
	public string SwaggerJson { get; set; }

	[Option('f', "file", Group = "input")]
	public string SwaggerFilePath { get; set; }
}

a new PR may Support Default Value for Group.

cc @hadzhiyski
It's nice if Group support Default value and firing error message when both SetName and Group are used in the same time.

[hadzhiyski]

@moh-hassan, @sm-g I will submit a new PR for this.

[moh-hassan]

Thanks @hadzhiyski for the quick response and support.

[hadzhiyski]

The new PR is #575