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").

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'

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

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

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,

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