Code review updates for deprecated_optionMIXLIB-CLI-63/deprecated-option-support

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

3 files changed, 55 insertions, 11 deletions

diff --git a/lib/mixlib/cli.rb b/lib/mixlib/cli.rb
index 5075dec..1a83a11 100644
--- a/lib/mixlib/cli.rb
+++ b/lib/mixlib/cli.rb
@@ -220,13 +220,6 @@ module Mixlib
     # the values supplied by the user, see #config.
     attr_accessor :options
 
-    # Warn if a duplicate item has been added to the options list.
-    # This is intended to notify developers when they're using
-    # mixlib-cli incorrectly - either by specifying an option more than once,
-    # or more commonly from re-specifying options that have been inhereted from a
-    # shared base class.
-    attr_accessor :warn_on_duplicates
-
     # A Hash containing the values supplied by command line options.
     #
     # The behavior and contents of this Hash vary depending on whether
@@ -396,7 +389,8 @@ module Mixlib
     #
     # @return NilClass
     def handle_deprecated_options(show_deprecations)
-      config.keys.each do |opt_key|
+      merge_in_values = {}
+      config.each_key do |opt_key|
         opt_cfg = options[opt_key]
         # Deprecated entries do not have defaults so no matter what
         # separate_default_options are set, if we see a 'config'
@@ -409,7 +403,10 @@ module Mixlib
           # the declared value mapper (defaults to return the same value if caller hasn't
           # provided a mapper).
           deprecated_val = config[opt_key]
-          config[replacement_key] = opt_cfg[:value_mapper].call(deprecated_val)
+
+          # We can't modify 'config' since we're iterating it, apply updates
+          # at the end.
+          merge_in_values[replacement_key] = opt_cfg[:value_mapper].call(deprecated_val)
           config.delete(opt_key) unless opt_cfg[:keep]
         end
 
@@ -420,14 +417,15 @@ module Mixlib
           puts "#{display_name}: #{opt_cfg[:description]}"
         end
       end
+      config.merge!(merge_in_values)
       nil
     end
 
     def build_option_arguments(opt_setting)
       arguments = Array.new
 
-      arguments << opt_setting[:short] unless opt_setting[:short].nil?
-      arguments << opt_setting[:long] unless opt_setting[:long].nil?
+      arguments << opt_setting[:short] if opt_setting[:short]
+      arguments << opt_setting[:long] if opt_setting[:long]
       if opt_setting.key?(:description)
         description = opt_setting[:description].dup
         description << " (required)" if opt_setting[:required]
diff --git a/lib/mixlib/cli/formatter.rb b/lib/mixlib/cli/formatter.rb
index b89090d..fef8503 100644
--- a/lib/mixlib/cli/formatter.rb
+++ b/lib/mixlib/cli/formatter.rb
@@ -10,6 +10,9 @@ module Mixlib
       # @return [String] the formatted flag name as described above
       def self.combined_option_display_name(short, long)
         usage = ""
+        # short/long may have an argument (--long ARG)
+        # splitting on " " and taking first ensures that we get just
+        # the flag name without the argument if one is present.
         usage << short.split(" ").first if short
         usage << "/" if long && short
         usage << long.split(" ").first if long
diff --git a/spec/mixlib/cli/formatter_spec.rb b/spec/mixlib/cli/formatter_spec.rb
new file mode 100644
index 0000000..5500528
--- /dev/null
+++ b/spec/mixlib/cli/formatter_spec.rb
@@ -0,0 +1,43 @@
+
+require "mixlib/cli/formatter"
+
+describe Mixlib::CLI::Formatter do
+  Formatter = Mixlib::CLI::Formatter
+  context "combined_option_display_name" do
+    it "converts --option with short -o to '-s/--option'" do
+      expect(Formatter.combined_option_display_name("-o", "--option")).to eql "-o/--option"
+    end
+
+    it "converts --option with no short to '--option'" do
+      expect(Formatter.combined_option_display_name(nil, "--option")).to eql "--option"
+    end
+    it "converts short -o with no long option to '-o'" do
+      expect(Formatter.combined_option_display_name("-o", nil)).to eql"-o"
+    end
+
+    it "converts options the same way even with an argument present" do
+      expect(Formatter.combined_option_display_name("-o arg1", "--option arg1")).to eql "-o/--option"
+    end
+
+    it "converts options to a blank string if neither short nor long are present" do
+      expect(Formatter.combined_option_display_name(nil, nil)).to eql ""
+    end
+  end
+
+  context "friendly_opt_list" do
+    it "for a single item it quotes it and returns it as a string" do
+      expect(Formatter.friendly_opt_list(%w{hello})).to eql "'hello'"
+    end
+    it "for two items returns ..." do
+      expect(Formatter.friendly_opt_list(%w{hello world})).to eql "'hello' or 'world'"
+    end
+    it "for three items returns..." do
+      expect(Formatter.friendly_opt_list(%w{hello green world})).to eql "'hello', 'green', or 'world'"
+    end
+    it "for more than three items creates a list in the same was as three items" do
+      expect(Formatter.friendly_opt_list(%w{hello green world good morning})).to eql "'hello', 'green', 'world', 'good', or 'morning'"
+    end
+
+  end
+
+end