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

Allow conditionally required members #112

Closed
wants to merge 5 commits into from
Closed

Allow conditionally required members #112

wants to merge 5 commits into from

Conversation

swagggpickle
Copy link

This PR introduces support for variables that maybe conditionally:

var args struct {
   UserName   string `default:"abc"`
   UserID     string `default:"123"`
   Password   string `arg:"required-if:username|userid"`
}
arg.MustParse(&args)

This will enable users to express the relationship between variables. This feature is most important for featured flagged capabilities where if a feature is turned on another variable should be set in order to work correctly.

@alexflint
Copy link
Owner

Thank for this PR @swagggpickle. However, I think this is probably too niche to justify adding it to the library. I believe it is possible to achieve the same using custom validation - would you agree?

@swagggpickle
Copy link
Author

Hi @alexflint, you are correct, it is possible to achieve the same behavior by using custom validation. I guess I was coming from the perspective that once MustParse() or Parse() has returned I would have liked to have as much validation done and not have to worry about whether or not all the variables have been set for specific feature or sub-command. Basically it would allow the user of this library to focus more on scrutinizing the values of the user input and allow the relationships between the variables to be defined in a more human readable fashion.

@alexflint
Copy link
Owner

@swagggpickle Yes there are definitely some nice things about having everything be done inside the call to Parse(). But there are costs to adding to the API, too, and in this case I don't think this change will be helpful enough to justify adding it to the API.

@swagggpickle
Copy link
Author

That is fair enough I will cose this PR then.

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.

None yet

2 participants