-
Notifications
You must be signed in to change notification settings - Fork 576
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
return result of GetOptionsFromArray in getopt(). #1343
Conversation
That way the users of getopt can check whether the option parsing was successful or not.
Overall I think it makes sense to return the result, meets with user expectations. For the tests, I wouldn't suggest testing the exact value but rather ok and ok not. Just check the boolean status. |
Agreed with @jberger. Also, what are the use cases for this feature? |
IMO, since we are explicitly (ie, documented as) wrapping Getopt::Long I don't especially think we need to consider use cases when conforming to the expectations of the library we wrap. If we wanted to have some other documented behavior then I'd think we'd need use-cases. |
The use case is this: When I implement an additional Mojo command I want to check if argument parsing succeeded or not. If the user provided wrong arguments I want to fail as early as possible because continuing makes no sense (and might be actually harmful). |
I wanted to merge this PR for the next release, but looks like it hasn't been fixed yet. |
I've replaced the is() checks with ok(). Anything else missing? |
Maybe an example in the docs? |
Will add a paragraph or two to the docs today... |
No, the documentation is fine, but the tests need descriptions to be consistent. |
I've made a branch that takes these commits, adds one to normalize the tests, and rebases against master (as of this comment). https://github.com/mojolicious/mojo/tree/getopt_return |
I've merged #1351 which encompasses and obviates this PR. |
That way the users of getopt can check whether the option parsing was successful or not.
Summary
Currently it's not possible to check e.g. in a ::Command whether the option parsing was successful or not. But if you want to exit as early as possible this is necessary.
Motivation
GetOptionsFromArray already has such a return value but it is not returned in getopt() as the Getopt::Long::Configure call is the last statement in the subroutine.