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

Permitted regexp/range support #158

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

nanobowers
Copy link
Collaborator

Improves on the permitted: feature from @akhoury6 by adding support for ranges and regexp's.
It also provides a permitted_response: to override the error message when the input is does not conform to expectations.
Some bugs were fixed for the Array case also (previously did not like integer arrays)

Tests for permitted from parser_test.rb were moved out into a separate file for ease of editing/grouping.

Improves #147

@miq-bot add-label enhancement
@miq-bot add-reviewer @Fryguy @akhoury6

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.

Overall LGTM - just a couple of minor things.

lib/optimist.rb Outdated Show resolved Hide resolved
case permitted
when nil then true
when Regexp then val.match permitted
when Range then permitted.to_a.map(&:to_s).include? val
Copy link
Member

Choose a reason for hiding this comment

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

This is a shame we can't use .include? directly on the Range. Something we might want to consider in the future somehow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

iirc I did this b/c of the typing mismatch between the given arg from the cmdline (always a string) and the range type, though maybe it could be compared the other way around?

Copy link
Member

Choose a reason for hiding this comment

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

Offhand, the way you did it is the only way I can think of, so overall I'm fine to merge. We get strings from the command line so there's really not much we can do about that.

fixed .match to .match?

Co-authored-by: Jason Frey <fryguy9@gmail.com>
@Fryguy Fryguy merged commit 93998cf into ManageIQ:master May 21, 2024
12 checks passed
@Fryguy Fryguy self-assigned this May 21, 2024
Fryguy added a commit that referenced this pull request Nov 11, 2024
Added:
- Align the short and long forms into their own columns in the help output (#145 - thanks akhoury6)
- Add support for DidYouMean when long options are spelled incorrectly (#150 - thanks nanobowers)
- Using `permitted:` restricts the allowed values that a end-user inputs to a pre-defined list (#147 - thanks akhoury6)
- Add exact_match to settings, defaulting to inexact matching (#154 - thanks nanobowers)
- Add setting to disable implicit short options (#155 - thanks nanobowers)
- Add alt longname and multiple char support (#151 - thanks nanobowers)
- Permitted regexp/range support (#158, #159- thanks nanobowers)
- Add some examples (#161 - thanks nanobowers)

Changed:
- Enable frozen_string_literal for future-ruby support (#149, #153  - thanks nanobowers)
- Refactor constraints (#156 - thanks nanobowers)
- Fix assert_raises to assert_raises_errmatch (#160 - thanks nanobowers)
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.

2 participants