I want to help contribute to arggroup so it can be merged with the master branch but I don't believe I currently understand the intended behavior of ArgGroup:validate. Hopefully I can get some clarification on the following:
Why is ArgGroup::validate taking a vector of names as apposed to iterating over the internal list directly?
If _required is false then the return value is always false. However, I think this behavior maybe wrong for EitherOf, or am I missing something?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Why is ArgGroup::validate taking a vector of names as apposed to iterating over the internal list directly?
The input is the list of arguments give to CmdLine::parse (i.e., basically argv). The internal list contains only the argument specification.
If _required is false then the return value is always false. However, I think this behavior maybe wrong for EitherOf, or am I missing something?
This is expected, validate returns true if (and only if) a required argument was missing. For EitherOf there is no required argument (thus it cannot be missing) since required is always false. That is, it is perfectly fine to specify none of the EitherOf args. OneOf on the other hand requires exactly one of the arguments and thus required is set to true. Basically OneOf is xorAdd and EitherOf is what you are asking for (if I understood it correctly).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thanks for the clarification. For some reason I completely missed the validate call in CmdLine::parse, now that I see the usage, things make sense.
Other than the missing "AllOf" and "AnyOf" implemenations (along with related tests), is there anything else that needs to be completed before merging with the master branch?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I wouldn't say either of those are strictly necessary as they don't provide any additional funcationality (it's mostly for completeness and eventual internal refactoring). It may be nice to add them just the same.
I guess a last look over the implementation to make sure nothing is fundamentally broken and maybe a bit of cleanup is what is needed before a merge. I would also like to think a bit about another feature request where custom ArgGroups (with various weird constraints) can be used to make sure there is nothing that prevents that in the future.
If you want to take a stab at adding All/AnyOf, please go ahead and I'll review and merge as necessary. If you want to start using them based on arggroup branch that is also fine, something that works like this should be merged to master at some point, perhaps with some internal details changed.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I want to help contribute to arggroup so it can be merged with the master branch but I don't believe I currently understand the intended behavior of ArgGroup:validate. Hopefully I can get some clarification on the following:
_requiredis false then the return value is always false. However, I think this behavior maybe wrong for EitherOf, or am I missing something?The input is the list of arguments give to CmdLine::parse (i.e., basically argv). The internal list contains only the argument specification.
This is expected, validate returns true if (and only if) a required argument was missing. For EitherOf there is no required argument (thus it cannot be missing) since required is always false. That is, it is perfectly fine to specify none of the EitherOf args. OneOf on the other hand requires exactly one of the arguments and thus required is set to true. Basically OneOf is xorAdd and EitherOf is what you are asking for (if I understood it correctly).
Thanks for the clarification. For some reason I completely missed the validate call in CmdLine::parse, now that I see the usage, things make sense.
Other than the missing "AllOf" and "AnyOf" implemenations (along with related tests), is there anything else that needs to be completed before merging with the master branch?
I wouldn't say either of those are strictly necessary as they don't provide any additional funcationality (it's mostly for completeness and eventual internal refactoring). It may be nice to add them just the same.
I guess a last look over the implementation to make sure nothing is fundamentally broken and maybe a bit of cleanup is what is needed before a merge. I would also like to think a bit about another feature request where custom ArgGroups (with various weird constraints) can be used to make sure there is nothing that prevents that in the future.
If you want to take a stab at adding All/AnyOf, please go ahead and I'll review and merge as necessary. If you want to start using them based on arggroup branch that is also fine, something that works like this should be merged to master at some point, perhaps with some internal details changed.