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 setting to disable implicit short options #155

Merged
merged 2 commits into from
May 10, 2024

Conversation

nanobowers
Copy link
Collaborator

In Optimist short options are automatically created for any given long option. (e.g: -f for --file)
Once enough options exist you are likely to have short-option name collisions, whereby you may compete for the short-option, and optimist must choose a backup short-letter e.g.

opt :file, "a filename", type: String
opt :format, "file format", type: String

Optimist wont allow collisions and thus it will go through letters to try to find a different unused letter. In this case -f for --file and -o for --format (because the f was already used). Unfortunately this makes the order of opt important. If you do this:

opt :format, "file format", type: String
opt :file, "a filename", type: String

then in this case you get -f for --format and -i for --file.

Often second choice short-arguments are not really what you want - it's not memorable. Additionally re-ordering or re-grouping options changes the short letter which is undesirable for users. IME, people tend to use the shortest means of enabling the option available so they get upset when you change the short-opt even though it's usually not as stable.

This can be fixed by using the short: argument for opt. To disable short options for an opt you can use short: :none. I often find that for less commonly used arguments I prefer to make the user call out the long form. This means setting short: :none on a lot of options. I prefer to set short: on every option to some value, so that I can ensure stability.

This PR allows you to disable the implicit short option generation for opt. You can use the setting explicit_short_opts: true and this will prevent the automatic creation of all short opts. Once this setting is applied, short opts can be enabled by explicitly adding short: '<letter>' to the opt.

Note that this implies that -h and -v for help/version are not automatically created when --help and --version are. I'm a little conflicted about this, as -h is very common for a help message but it feels inconsistent to not do so. the net result is explicit definition of opt :help in order to get both --help/-h. Feedback appreciated.

If you don't add the explicit_short_opts: true, the present behavior of Optimist is unchanged.

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

@Fryguy
Copy link
Member

Fryguy commented May 7, 2024

Note that this implies that -h and -v for help/version are not automatically created when --help and --version are. I'm a little conflicted about this, as -h is very common for a help message but it feels inconsistent to not do so. the net result is explicit definition of opt :help in order to get both --help/-h. Feedback appreciated.

That is indeed tricky. I'd probably be ok with merging as is without that.

Spitballing an idea, I was thinking what if we had something to "edit" a previously defined opt? That way it wouldn't replace the opt, but would merge into it. Roughly:

opt_override :help, :short => "h"

This would also allow you to "plugin" things that could override or even remove options. In one of our gems, I provide helper methods for building CLIs with common options (for example, --dry-run, -n), and it looks like:

opts = Options.options do
  opt :foo

  MultiRepo::CLI.common_options(self)
end

What I can't do is modify those options after they've been added - I would love to do something like:

opts = Options.options do
  opt :foo

  MultiRepo::CLI.common_options(self)
  opt_override :dry_run, :default => true
end

Thoughts?

lib/optimist.rb Outdated
DEFAULT_SETTINGS = { suggestions: true, exact_match: true }
DEFAULT_SETTINGS = {
exact_match: true,
explicit_short_opts: false,
Copy link
Member

@Fryguy Fryguy May 7, 2024

Choose a reason for hiding this comment

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

I'm finding the "negative" option text, a default of false, and then using unless in the code with it very confusing (like a triple negative in my mind). Perhaps it's better to flip the logic to the positive with:

Suggested change
explicit_short_opts: false,
implicit_short_opts: true,

Then later, for example, this reads better to me:

-     resolve_default_short_options! unless @settings[:explicit_short_opts]
+     resolve_default_short_options! if @settings[:implicit_short_opts]

Copy link
Member

Choose a reason for hiding this comment

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

This would also set all the default settings to "true" and making them all "opt-out", so it's consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll switch the polarity from explicit_short_opts: false to implicit_short_opts: true and push it up

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 - have one question about the explicit vs implicit.

@Fryguy Fryguy self-assigned this May 8, 2024
@nanobowers
Copy link
Collaborator Author

Spitballing an idea, I was thinking what if we had something to "edit" a previously defined opt? That way it wouldn't replace the opt, but would merge into it. Roughly:

opt_override :help, :short => "h"

This would also allow you to "plugin" things that could override or even remove options. In one of our gems, I provide helper methods for building CLIs with common options (for example, --dry-run, -n), and it looks like:

opts = Options.options do
  opt :foo

  MultiRepo::CLI.common_options(self)
end

What I can't do is modify those options after they've been added - I would love to do something like:

opts = Options.options do
  opt :foo

  MultiRepo::CLI.common_options(self)
  opt_override :dry_run, :default => true
end

Thoughts?

This is a neat idea, but i'm not sure it can easily solve the problem with hacking the opts created for help/version since they're autovivified inside the parse method. At least the way I envision your opt_override working, it would effectively find and edit an opt command that was already given inside the block. I think it could work if opt_override worked with a delayed evaluation, but that's a little harder to reason with.

@Fryguy
Copy link
Member

Fryguy commented May 10, 2024

opts created for help/version since they're autovivified inside the parse method

Oh I misread the code...I didn't realize the block was executed before the opt :help line. Yeah, we'd have to rearrange that where the help and version happens first, but that will create other confusions. 😕

@Fryguy Fryguy merged commit 31b1099 into ManageIQ:master May 10, 2024
12 checks passed
@Fryguy Fryguy changed the title added settings to only create short options explicitly Added setting to disable implicit short options Nov 11, 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