Introduce option groups#552
Introduce option groups#552moh-hassan merged 14 commits intocommandlineparser:developfrom hadzhiyski:feature/group-options
Conversation
When one or more options has group set, at least one of these properties should have set value (they behave as required)
|
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 |
|
Thanks @hadzhiyski for your work. As I understand group (correct me):
The following use case using 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: This is not valid: Can this approach help for your use case? |
|
Hello, This is a useful solution. However it's not exactly what I am trying to achieve. But Mutually exclusive sets allow me use only one of the two options, but never together. How does this sound to you? I hope it makes sense. |
|
I give it a quick run test.It seems it's useful and good.
|
|
@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? |
|
You can follow the instructions of How to contribute to Wiki Documentation and attach the git patch file in PR. |
|
Hello, |
|
|
Thanks @hadzhiyski for the update. There are some missing unit tests:
Need a unit test
Group should take Default into account.
Edit. |
@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?
I also think the best way will be to raise error. |
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. You are free to implement this rule. |
|
@hadzhiyski |
There was a problem hiding this comment.
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 SetNamewhich 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
left a comment
There was a problem hiding this comment.
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>"
|
@moh-hassan Thanks for the review. |
|
PR is updated by the commit: Modify type of Group to string instead of `Maybe' |
|
Brilliant. Thank you so much! |
|
@moh-hassan Thank you for the great work! |
|
@moh-hassan cli is confusing now for such options: when running exe without any args we have output: So, default value for url will be set only when exe invoked with Also new Group feature does not prevent setting default value for "mutually exclusive" properties, it that by design? |
|
@sm-g 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 |
|
@moh-hassan, @sm-g I will submit a new PR for this. |
|
Thanks @hadzhiyski for the quick response and support. |
|
The new PR is #575 |
I found a missing use case in the
commandlinelibrary.When one or more options has group set, at least one of these properties should have set value (they behave as required)
Example:
I hope this makes sense.
Closes #546