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

Add alt longname and multiple char support #151

Merged
merged 2 commits into from May 11, 2024

Conversation

nanobowers
Copy link
Collaborator

It can be useful to have more than one option name for the same option. E.g. you may have a --concat option, but intuitively to some users they would rather --append. Or sometimes you want to change names of the option but keep backward compatibility.
This enables short options and long options to have more than one name.

For short options, the chars: can now take a list of strings/symbols (previously was just a symbol or char)
For long options, the alt: takes additional alternate long options. The value provided in long: is still respected, any name(s) given in alt: are used in addition to the default long-argument or what was provided in long:

There was some refactoring of how short and long options work, which pushed them down into a ShortNames and LongNames class. This caused an update a couple of existing tests. I think this is fine unless someone out there was accessing internals of the Option class, probing the short and long instance variables and expecting them to be strings.

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

@@ -636,7 +708,10 @@ def parse(_paramlist, _neg_given)
def type_format ; "" ; end

def educate
(short? ? "-#{short}, " : " ") + "--#{long}" + type_format + (flag? && default ? ", --no-#{long}" : "")
optionlist = []
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noting that this is an undo of the short-argument padding/alignment.
If there can be multiple short args, then the padding doesnt make sense anymore or it would be a bit more complicated to align.

@Fryguy Fryguy self-assigned this May 1, 2024
Comment on lines +649 to +724
if sopt =~ INVALID_ARG_REGEX
raise ArgumentError, "short option name '#{sopt}' can't be a number or a dash"
end
Copy link
Member

Choose a reason for hiding this comment

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

numeric options are interesting though. I'm reminded of git log -1 - would be cool to support a singular pure number as some sort of new type. 🤔

For this piece of code though I agree that the short opt can't be a number

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a reformat of this

raise ArgumentError, "a short option name can't be a number or a dash" if sopt =~ ::Optimist::Parser::INVALID_SHORT_ARG_REGEX

but yeah I agree. Implementing commands like ls (e.g. ls -1 or ls -d1) with optimist is impractical as-is due to the numerical short-opt restriction. I found this out trying to implement a version of colorls some time back. OTOH, there are unintended consequences to supporting this, bc it's harder to tell what's a negative integer vs. an option, especially since optmist allows ambiguous integer lists.

lib/optimist.rb Outdated Show resolved Hide resolved
@auto = false
return
end
raise ArgumentError, "Cannot use :none with any other values in short option: #{values.inspect}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to prevent :none from being used with any other value (including itself)

@Fryguy
Copy link
Member

Fryguy commented May 10, 2024

Gah I'm sorry - I keep merging the other ones and this keeps conflicting 😅

@Fryguy Fryguy merged commit 35c8bf4 into ManageIQ:master May 11, 2024
12 checks passed
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.

None yet

2 participants