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

return result of GetOptionsFromArray in getopt(). #1343

Closed
wants to merge 2 commits into from

Conversation

reneeb
Copy link
Contributor

@reneeb reneeb commented Apr 24, 2019

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.

That way the users of getopt can check whether the option parsing was successful or not.
@jberger
Copy link
Member

jberger commented Apr 24, 2019

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.

@kraih
Copy link
Member

kraih commented Apr 24, 2019

Agreed with @jberger. Also, what are the use cases for this feature?

@jberger
Copy link
Member

jberger commented Apr 24, 2019

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.

@glauschwuffel
Copy link

Agreed with @jberger. Also, what are the use cases for this feature?

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).

@kraih
Copy link
Member

kraih commented Apr 26, 2019

I wanted to merge this PR for the next release, but looks like it hasn't been fixed yet.

@reneeb
Copy link
Contributor Author

reneeb commented Apr 29, 2019

I've replaced the is() checks with ok(). Anything else missing?

@Tekki
Copy link

Tekki commented Apr 30, 2019

Anything else missing?

Maybe an example in the docs?

@reneeb
Copy link
Contributor Author

reneeb commented Apr 30, 2019

Will add a paragraph or two to the docs today...

@kraih
Copy link
Member

kraih commented Apr 30, 2019

No, the documentation is fine, but the tests need descriptions to be consistent.

@jberger
Copy link
Member

jberger commented May 7, 2019

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

@jberger jberger mentioned this pull request May 10, 2019
@jberger
Copy link
Member

jberger commented May 13, 2019

I've merged #1351 which encompasses and obviates this PR.

@jberger jberger closed this May 13, 2019
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.

5 participants