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

Added parse_and_check/2 function to verify all required options #22

Merged
merged 1 commit into from
Jul 18, 2013
Merged

Added parse_and_check/2 function to verify all required options #22

merged 1 commit into from
Jul 18, 2013

Conversation

saleyn
Copy link
Contributor

@saleyn saleyn commented Jul 16, 2013

This patch adds a new function to verify that all required options have been given values.
It is a backward compatible change.

@jcomellas
Copy link
Owner

@saleyn Thanks for the patch. I had thought about adding checks for required arguments before, but I wasn't sure I could assume that arguments that don't have default values must always be present. That's why I left the task of checking the arguments to the user. The main issue I have with the patch is that when a required argument is missing, an exception is thrown instead of just returning an error. Also, I think we should return the atom with the name of the missing option instead of its command-line representation, as this error is going to be received by the application and not the end user.

If we were to include this functionality, I think it might be better to just add an additional function called check/2 that receives the list of parsed options returned by parse/2. What do you think?

@saleyn
Copy link
Contributor Author

saleyn commented Jul 18, 2013

Can you think of a case when an argument is not given a default should be treated as non-required? It seems to me that it's always the case that you want to specify default behavior in one place as opposed to scattering it throughout the code, that's why I think it is a good assumption. I will rework the patch to split it in two functions check/2 and parse_and_check/2 that don't throw. This way it would give the user all control.

@jcomellas
Copy link
Owner

Normally, if it's an option, it should have a value, so in that sense I agree with you. It's just that the user might choose not to give it a default value in the getopt option specifications and set the value in some other way.

@saleyn
Copy link
Contributor Author

saleyn commented Jul 18, 2013

Which is what we call the "default" behavior. ;-)
Perhaps in case when a type is specified, the default value could be a function, which would cover the case you mention that parse/2 would call that function to determine the value, so that it's not fixed, and not required?

@jcomellas
Copy link
Owner

Yes, it's a possible solution, but I think it's overkill for this particular case, as I'd prefer to keep the option specifications as "declarative" as possible. I also tried to add option validation a long time ago, but it becomes so complex so fast, that the only sane way to cover all the cases is to allow the user to provide a validation function for each option. I finally reached the conclusion that this problem was out of getopt's scope.

@saleyn
Copy link
Contributor Author

saleyn commented Jul 18, 2013

Ok, I pushed the changes we discussed. This version gives the developer control over required and optional argument checking and also adds the error formatting function for reporting.

jcomellas added a commit that referenced this pull request Jul 18, 2013
Added check/2 and parse_and_check/2 functions to verify all required options
@jcomellas jcomellas merged commit 3699279 into jcomellas:master Jul 18, 2013
@jcomellas
Copy link
Owner

OK, I'll merge the patch, but for consistency I will change getopt:check/2 to receive the list of option specifications as the first argument. Thanks!

@saleyn
Copy link
Contributor Author

saleyn commented Jul 19, 2013

Thanks! I also just checked in the test cases for new functions.

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