Refactor option handling internals

Currently we handle options in several ways, and it is hard to remember
what code needs an option string ("--foo"), what needs only the name
("foo") and what needs an Option object.

Now that Option objects can act as strings and be converted to JSON, we
can start using them instead of passing around strings between Formula
objects, Tab objects, and ARGV-style arrays.

The Options class is a special collection that can be queried for the
inclusion of options in any form: '--foo', 'foo', or Option.new("foo").

5 files changed, 188 insertions, 43 deletions

diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb
index 1c312ddfa..0df987f1f 100644
--- a/Library/Homebrew/formula_installer.rb
+++ b/Library/Homebrew/formula_installer.rb
@@ -208,7 +208,7 @@ class FormulaInstaller
     ENV['HOMEBREW_ERROR_PIPE'] = write.to_i.to_s
 
     args = ARGV.clone
-    args.concat tab.used_options unless tab.nil? or args.include? '--fresh'
+    args.concat(tab.used_options.as_flags) unless tab.nil? or args.include? '--fresh'
     # FIXME: enforce the download of the non-bottled package
     # in the spawned Ruby process.
     args << '--build-from-source'
diff --git a/Library/Homebrew/formula_support.rb b/Library/Homebrew/formula_support.rb
index be50b0138..52fe42e03 100644
--- a/Library/Homebrew/formula_support.rb
+++ b/Library/Homebrew/formula_support.rb
@@ -1,6 +1,7 @@
 require 'download_strategy'
 require 'checksums'
 require 'version'
+require 'options'
 
 class SoftwareSpec
   attr_reader :checksum, :mirrors, :specs
@@ -161,36 +162,6 @@ class KegOnlyReason
   end
 end
 
-
-# Represents a build-time option for a formula
-class Option
-  attr_reader :name, :description, :flag
-
-  def initialize name, description=nil
-    @name = name.to_s
-    @description = description.to_s
-    @flag = '--'+name.to_s
-  end
-
-  def to_s
-    flag
-  end
-  alias_method :to_str, :to_s
-
-  def to_json
-    flag.inspect
-  end
-
-  def eql?(other)
-    @name == other.name
-  end
-
-  def hash
-    @name.hash
-  end
-end
-
-
 # This class holds the build-time options defined for a Formula,
 # and provides named access to those options during install.
 class BuildOptions
@@ -198,11 +169,8 @@ class BuildOptions
   include Enumerable
 
   def initialize args
-    # Take a copy of the args (any string array, actually)
-    @args = Array.new(args)
-    # Extend it into an ARGV extension
-    @args.extend(HomebrewArgvExtension)
-    @options = Set.new
+    @args = Array.new(args).extend(HomebrewArgvExtension)
+    @options = Options.new
   end
 
   def add name, description=nil
@@ -222,12 +190,12 @@ class BuildOptions
     @options.empty?
   end
 
-  def each(&blk)
-    @options.each(&blk)
+  def each(*args, &block)
+    @options.each(*args, &block)
   end
 
   def as_flags
-    map { |opt| opt.flag }
+    @options.as_flags
   end
 
   def include? name
@@ -259,10 +227,10 @@ class BuildOptions
   end
 
   def used_options
-    as_flags & @args.options_only
+    Options.new((as_flags & @args.options_only).map { |o| Option.new(o) })
   end
 
   def unused_options
-    as_flags - @args.options_only
+    Options.new((as_flags - @args.options_only).map { |o| Option.new(o) })
   end
 end
diff --git a/Library/Homebrew/options.rb b/Library/Homebrew/options.rb
new file mode 100644
index 000000000..10abc1fae
--- /dev/null
+++ b/Library/Homebrew/options.rb
@@ -0,0 +1,78 @@
+class Option
+  include Comparable
+
+  attr_reader :name, :description, :flag
+
+  def initialize(name, description=nil)
+    @name = name.to_s[/^(?:--)?(.+)$/, 1]
+    @description = description.to_s
+    @flag = "--#{@name}"
+  end
+
+  def to_s
+    flag
+  end
+  alias_method :to_str, :to_s
+
+  def to_json
+    flag.inspect
+  end
+
+  def <=>(other)
+    name <=> other.name
+  end
+
+  def eql?(other)
+    other.is_a?(self.class) && hash == other.hash
+  end
+
+  def hash
+    name.hash
+  end
+end
+
+class Options
+  include Enumerable
+
+  def initialize(*args)
+    @options = Set.new(*args)
+  end
+
+  def each(*args, &block)
+    @options.each(*args, &block)
+  end
+
+  def <<(o)
+    @options << o
+    self
+  end
+
+  def +(o)
+    Options.new(@options + o)
+  end
+
+  def -(o)
+    Options.new(@options - o)
+  end
+
+  def *(arg)
+    @options.to_a * arg
+  end
+
+  def empty?
+    @options.empty?
+  end
+
+  def as_flags
+    map(&:flag)
+  end
+
+  def include?(o)
+    any? { |opt| opt == o || opt.name == o || opt.flag == o }
+  end
+
+  def to_a
+    @options.to_a
+  end
+  alias_method :to_ary, :to_a
+end
diff --git a/Library/Homebrew/tab.rb b/Library/Homebrew/tab.rb
index 7e229393f..6735efe5d 100644
--- a/Library/Homebrew/tab.rb
+++ b/Library/Homebrew/tab.rb
@@ -1,5 +1,6 @@
 require 'ostruct'
 require 'formula'
+require 'options'
 require 'vendor/multi_json'
 
 # Inherit from OpenStruct to gain a generic initialization method that takes a
@@ -61,14 +62,22 @@ class Tab < OpenStruct
     used_options.include? opt
   end
 
+  def used_options
+    Options.new(super.map { |o| Option.new(o) })
+  end
+
+  def unused_options
+    Options.new(super.map { |o| Option.new(o) })
+  end
+
   def options
     used_options + unused_options
   end
 
   def to_json
     MultiJson.encode({
-      :used_options => used_options,
-      :unused_options => unused_options,
+      :used_options => used_options.to_a,
+      :unused_options => unused_options.to_a,
       :built_as_bottle => built_as_bottle,
       :tapped_from => tapped_from,
       :time => time,
diff --git a/Library/Homebrew/test/test_options.rb b/Library/Homebrew/test/test_options.rb
new file mode 100644
index 000000000..065c00a82
--- /dev/null
+++ b/Library/Homebrew/test/test_options.rb
@@ -0,0 +1,90 @@
+require 'testing_env'
+require 'options'
+
+class OptionTests < Test::Unit::TestCase
+  def setup
+    @option = Option.new("foo")
+  end
+
+  def test_to_s
+    assert_equal "--foo", @option.to_s
+  end
+
+  def test_to_str
+    assert_equal "--foo", @option.to_str
+  end
+
+  def test_to_json
+    assert_equal %q{"--foo"}, @option.to_json
+  end
+
+  def test_equality
+    foo = Option.new("foo")
+    bar = Option.new("bar")
+    assert_equal foo, @option
+    assert_not_equal bar, @option
+    assert @option.eql?(foo)
+    assert !@option.eql?(bar)
+    assert bar < foo
+  end
+
+  def test_strips_leading_dashes
+    option = Option.new("--foo")
+    assert_equal "foo", option.name
+    assert_equal "--foo", option.flag
+  end
+
+  def test_description
+    assert_empty @option.description
+    assert_equal "foo", Option.new("foo", "foo").description
+  end
+end
+
+class OptionsTests < Test::Unit::TestCase
+  def setup
+    @options = Options.new
+  end
+
+  def test_no_duplicate_options
+    @options << Option.new("foo")
+    @options << Option.new("foo")
+    assert @options.include? "--foo"
+    assert_equal 1, @options.count
+  end
+
+  def test_include
+    @options << Option.new("foo")
+    assert @options.include? "--foo"
+    assert @options.include? "foo"
+    assert @options.include? Option.new("foo")
+  end
+
+  def test_union_returns_options
+    assert_instance_of Options, (@options + Options.new)
+  end
+
+  def test_difference_returns_options
+    assert_instance_of Options, (@options - Options.new)
+  end
+
+  def test_shovel_returns_self
+    assert_same @options, (@options << Option.new("foo"))
+  end
+
+  def test_as_flags
+    @options << Option.new("foo")
+    assert_equal %w{--foo}, @options.as_flags
+  end
+
+  def test_to_a
+    option = Option.new("foo")
+    @options << option
+    assert_equal [option], @options.to_a
+  end
+
+  def test_to_ary
+    option = Option.new("foo")
+    @options << option
+    assert_equal [option], @options.to_ary
+  end
+end