Improved formula attribute validation

The initializer for Formula does a number of validations, but it does
them in a weird order, and some attributes aren't validated under
certain circumstances. This became even more of a mess when most
software package attributes were moved into the SoftwareSpec class.

This commit removes the last vestiges of storing these attributes as
instance variables. In particular, it eliminates #set_instance_variable
and #validate_variable, replacing them with methods that operate on
SoftwareSpec instances, and generate more useful errors.

Doing these validations unconditionally in the initializer means we bail
out much earlier if the formula has invalid attributes or is not fully
specified, and no longer need to validate in #prefix.

Technically we don't need to validate in #brew either, but we continue
to do so anyway as a safety measure, and because we cannot enforce calls
to super in subclasses.

3 files changed, 70 insertions, 26 deletions

diff --git a/Library/Homebrew/test/test_formula.rb b/Library/Homebrew/test/test_formula.rb
index 30ef41589..434dd3ac1 100644
--- a/Library/Homebrew/test/test_formula.rb
+++ b/Library/Homebrew/test/test_formula.rb
@@ -1,10 +1,6 @@
 require 'testing_env'
 require 'test/testball'
 
-class MostlyAbstractFormula < Formula
-  url ''
-end
-
 class FormulaTests < Test::Unit::TestCase
   include VersionAssertions
 
@@ -58,19 +54,6 @@ class FormulaTests < Test::Unit::TestCase
     assert_equal 'FooBar', Formula.class_s('foo_bar')
   end
 
-  def test_cant_override_brew
-    assert_raises(RuntimeError) do
-      Class.new(Formula) { def brew; end }
-    end
-  end
-
-  def test_abstract_formula
-    f=MostlyAbstractFormula.new
-    assert_equal '__UNKNOWN__', f.name
-    assert_raises(RuntimeError) { f.prefix }
-    shutup { assert_raises(RuntimeError) { f.brew } }
-  end
-
   def test_mirror_support
     f = Class.new(Formula) do
       url "file:///#{TEST_FOLDER}/bad_url/testball-0.1.tbz"
@@ -286,7 +269,7 @@ class FormulaTests < Test::Unit::TestCase
       f << %{
         require 'formula'
         class #{Formula.class_s(foobar)} < Formula
-          url ''
+          url 'foo-1.0'
           def initialize(*args)
             @homepage = 'http://example.com/'
             super
diff --git a/Library/Homebrew/test/test_formula_validation.rb b/Library/Homebrew/test/test_formula_validation.rb
new file mode 100644
index 000000000..be4c38c64
--- /dev/null
+++ b/Library/Homebrew/test/test_formula_validation.rb
@@ -0,0 +1,69 @@
+require 'testing_env'
+require 'formula'
+
+class FormulaValidationTests < Test::Unit::TestCase
+  def formula(*args, &block)
+    Class.new(Formula, &block).new(*args)
+  end
+
+  def assert_invalid(attr, &block)
+    e = assert_raises(FormulaValidationError, &block)
+    assert_equal attr, e.attr
+  end
+
+  def test_cant_override_brew
+    assert_raises(RuntimeError) do
+      Class.new(Formula) { def brew; end }
+    end
+  end
+
+  def test_validates_name
+    assert_invalid :name do
+      formula "name with spaces" do
+        url "foo"
+        version "1.0"
+      end
+    end
+  end
+
+  def test_validates_url
+    assert_invalid :url do
+      formula do
+        url ""
+        version "1"
+      end
+    end
+  end
+
+  def test_validates_version
+    assert_invalid :version do
+      formula do
+        url "foo"
+        version "version with spaces"
+      end
+    end
+
+    assert_invalid :version do
+      formula do
+        url "foo"
+        version ""
+      end
+    end
+  end
+
+  def test_validates_when_initialize_overridden
+    assert_invalid :name do
+      formula do
+        def initialize; end
+      end.brew {}
+    end
+  end
+
+  def test_head_only_valid
+    assert_nothing_raised do
+      formula do
+        head "foo"
+      end
+    end
+  end
+end
diff --git a/Library/Homebrew/test/test_versions.rb b/Library/Homebrew/test/test_versions.rb
index d503f4ace..bf943637e 100644
--- a/Library/Homebrew/test/test_versions.rb
+++ b/Library/Homebrew/test/test_versions.rb
@@ -50,14 +50,6 @@ class VersionParsingTests < Test::Unit::TestCase
     assert_version_nil 'foo'
   end
 
-  def test_bad_version
-    assert_raises(RuntimeError) do
-      Class.new(TestBall) do
-        version "versions can't have spaces"
-      end.new
-    end
-  end
-
   def test_version_all_dots
     assert_version_detected '1.14', 'http://example.com/foo.bar.la.1.14.zip'
   end