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

Using permitted: restricts the allowed values that a end-user inputs to a pre-defined list #147

Merged
merged 2 commits into from
May 1, 2024

Conversation

akhoury6
Copy link
Contributor

@akhoury6 akhoury6 commented Apr 23, 2024

Using permitted: when defining an option restricts the allowed values that a end-user inputs to a pre-defined list.

Note: This is a continuation of an old PR which I accidentally closed by deleting my old fork (it was too old to use now). This is the PR. It is now rebased as requested, and I have added the tests, but I never got a final answer if you wanted it merged in this form or if you wanted to do something else. I've been using my own fork for the last few years and so far I haven't hit any issues with the code as written.

Defining an option like this...

opt :name, type: strings, permitted: ['Jack', 'Jill']

...restricts the input to one of the following:

$ mycmd -n Jack
$ mycmd -n Jill
$ mycmd -n Jack Jill

Any other input will raise a CommandlineError in line with other features.

…to a pre-defined list.

For example:

opt :name, type: string, permitted: ['Jack', 'Jill']

means the user can only do one of the following:

$ mycmd -n Jack
$ mycmd -n Jill

It also works for multi-parameters

$ mycmd -n Jack Jill
@Fryguy Fryguy self-assigned this Apr 24, 2024
@Fryguy
Copy link
Member

Fryguy commented Apr 25, 2024

Can you show an example of what --help would show? Otherwise this looks like a really nice change.

@@ -158,6 +158,17 @@ def test_help_has_grammatical_default_text
assert help[1] =~ /Default/
assert help[2] =~ /default/
end

def test_help_has_grammatical_permitted_text
parser.opt :arg1, 'description with period.', :type => :strings, :permitted => %w(foo bar)
Copy link
Member

Choose a reason for hiding this comment

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

Does this also work with other types that are not strings? At a minimum I was thinking :string type, but even something like an integer would be useful to constrain. If we can have other types constrained, I think we should also have some specs for those.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@akhoury6 @Fryguy This functionality is was added in Optimist_XL (based on your code/idea that I merged in), see:

Along with some other enhancements to the idea including range / regex matching and permitted_response:

I do get that some of the optimist-xl features aren't for everyone, but if you decide to merge this permitted: feature in it may be worth considering borrowing code (or at least the tests) from Optimist_XL

Copy link
Contributor Author

@akhoury6 akhoury6 Apr 26, 2024

Choose a reason for hiding this comment

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

If I had known you had an OptimistXL I would have just used that instead! For this and the other PRs too.

@Fryguy That's just a test, the code should work with all types. See Line 334.

@akhoury6
Copy link
Contributor Author

Can you show an example of what --help would show? Otherwise this looks like a really nice change.

Options:
  -s, --something=<s>    Description (permitted: foo, bar)
  -h, --help             Show this message

@nanobowers
Copy link
Collaborator

Can you show an example of what --help would show? Otherwise this looks like a really nice change.

Options:
  -s, --something=<s>    Description (permitted: foo, bar)
  -h, --help             Show this message

Here's an example from optmist-xl. I must not have tested the --help on permitted range until now - it's kind of a mess and needs help. The way the regex types print is a little odd, but how Ruby does it, suggestions for improvement appreciated.

07:34:02 $ ./permitted.rb -h
Options:
  -f, --french=<s>     starts with french (Permitted: fries, toast)
  -d, --dog=<s>        starts with dog (Permitted: (?-mix:(house|bone|tail)))
  -z, --zipcode=<s>    zipcode (Default: 39759) (Permitted: (?-mix:^[0-9]{5}$))
  -a, --age            adult age (Permitted: 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28,
                       29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45,
                       46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62,
                       63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
                       80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96,
                       97, 98, 99)
  -h, --help           Show this message

@akhoury6
Copy link
Contributor Author

akhoury6 commented Apr 26, 2024

When you try to print a regex like that you're using the /regex/.to_s method with produces that sort of garbage, but if you use /regex/.inspect it generally gives you the original regex that you typed. Inspect will do funny things to other classes though like add quotes around strings, so permitted.inspect if permitted.is_a?(Regexp) is likely the way to go.

As for that long list of numbers... that one's a bit harder, but I can think of two ways to do it. Either

  • create a method "try_to_shorten()" that identifies ranges and shortens them to a string like 18 to 99 by specifically looking for sequences like that in an array, say of 3 or more in a sequence. Or...
  • have the application try to parse the source code itself to get what the programmer typed in. You can get the calling line using caller_locations from within the opt method when it is declared by the programmer, then parse the line like /permitted: (.*)/. The parsing will have to be smarter than that but you get the idea.

Edit: https://stackoverflow.com/questions/20847212/how-to-convert-an-array-of-number-into-ranges

@Fryguy
Copy link
Member

Fryguy commented Apr 26, 2024

@nanobowers I believe you have merge rights here now, so I'll leave it up to you how you would like to proceed. Personally I'm ok merging this PR as a first pass, then moving to the more thorough optimist xl version afterwards.

@Fryguy
Copy link
Member

Fryguy commented Apr 26, 2024

@akhoury6 Also note that optimist_xl has a thing called permitted_response, which allows you to give human words instead of a straight printout of the list/regex/range.

@nanobowers
Copy link
Collaborator

@nanobowers I believe you have merge rights here now, so I'll leave it up to you how you would like to proceed. Personally I'm ok merging this PR as a first pass, then moving to the more thorough optimist xl version afterwards.

@Fryguy Yeah that sounds good, lets get this merged. I can add in the additional features related to this (range/regex support and permitted_response:) in a separate enhancement PR.

@Fryguy Fryguy merged commit 3757b70 into ManageIQ:master May 1, 2024
11 of 12 checks passed
@Fryguy
Copy link
Member

Fryguy commented May 1, 2024

@nanobowers @akhoury6 I merged this one after #149 , but didn't have it rerun the specs and now it's broken on Ruby 2.7. Can you take a look?

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.

3 participants