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

implement either :one, :two #130

Merged
merged 2 commits into from
Apr 19, 2022
Merged

implement either :one, :two #130

merged 2 commits into from
Apr 19, 2022

Conversation

alezummo
Copy link
Contributor

Makes at least one of the opts required, but fails if both or none are given.

lib/optimist.rb Outdated Show resolved Hide resolved
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - a small comment about preferring interpolation over concatenation. @kbrock Please also review.

Co-authored-by: Jason Frey <fryguy9@gmail.com>
@nanobowers
Copy link
Collaborator

nanobowers commented Apr 16, 2022

This seems like a specialization of a more general type of argument validation.

More specifically, the condition you are trying to satisfy with either: is max==1 and min==1. Should we entertain cases where min/max are not 1? conflicts: is kinda like max==1 and min==ANY

Was thinking something along the lines of:

validate min: <Integer/nil>, max: <Integer/nil>, args: <Array of Symbol>

Another related thought - @Fryguy should there any default notation in the help message for exclusivity clauses? If the developer uses either:, conflicts: or depends, seems like the only good way to find out is to try it and get an error message, or the developer would have to manually note it with the banner message.

@Fryguy
Copy link
Member

Fryguy commented Apr 16, 2022

While it might be nice to have a general case, that doesn't preclude having specialized methods, which are readable, and can later be implemented in terms of the generic code. conflicts :bar, :baz is probably clearer than something like constraint :bar, :baz, min: 1, max: :any.

For the help text I was thinking the same thing, but since depends and conflicts don't currently do that, then I don't think it should hold up this PR. I'm also not exactly sure how one would implement that.

@alezummo
Copy link
Contributor Author

alezummo commented Apr 16, 2022

I particularly liked the elegance of writing either instead of a more convoluted syntax.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed there are no tests for the happy path.

Amusingly, typically people just test the working code and forget to test the problems. Seems you have done the opposite.

Just add a working example and then lets shipit

@Fryguy
Copy link
Member

Fryguy commented Apr 19, 2022

@kbrock I think the test lines that are calling .parse, but not wrapped in assert_raises are the happy path. i.e. https://github.com/ManageIQ/optimist/pull/130/files#diff-2ba0f24469c763ceb2f38630ff0526bef69b594e08daa3c1bcd12889a14141beR771-R772 and https://github.com/ManageIQ/optimist/pull/130/files#diff-2ba0f24469c763ceb2f38630ff0526bef69b594e08daa3c1bcd12889a14141beR785-R788

@kbrock kbrock assigned Fryguy and unassigned kbrock Apr 19, 2022
@kbrock
Copy link
Member

kbrock commented Apr 19, 2022

@Fryguy LGTM. merge if you are all set

@Fryguy Fryguy merged commit 31cb8e2 into ManageIQ:master Apr 19, 2022
Fryguy added a commit that referenced this pull request Jul 24, 2023
Added
- Implement `either` command (#130)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants