Clarify logic and error messages when initializing Puppet::FileBucket::File

Rather than stating the logic as 'if !thing', the two checks done when
initializing a new Puppet::FileBucket::File are now phrased as 'unless
thing', which should lessen the likelihood of overlooking the '!'.

We also now provide a reason for the ArgumentError being raised, which
should help users of Puppet::FileBucket::File quickly figure out what
is the problem when these exceptions are raised.

In addition to updating the tests to look for these new error
messages, we update the existing tests to specify which type of
exception, and what message it should have, when something is raised.

Reviewed-by: Nick Lewis <nick@puppetlabs.com>

2 files changed, 12 insertions, 6 deletions

diff --git a/lib/puppet/file_bucket/file.rb b/lib/puppet/file_bucket/file.rb
index 08c0329f1..2a0558fde 100644
--- a/lib/puppet/file_bucket/file.rb
+++ b/lib/puppet/file_bucket/file.rb
@@ -15,11 +15,11 @@ class Puppet::FileBucket::File
   attr :bucket_path
 
   def initialize( contents, options = {} )
-    raise ArgumentError if !contents.is_a?(String)
-    @contents  = contents
+    raise ArgumentError.new("contents must be a String, got a #{contents.class}") unless contents.is_a?(String)
+    @contents = contents
 
     @bucket_path = options.delete(:bucket_path)
-    raise ArgumentError if options != {}
+    raise ArgumentError.new("Unknown option(s): #{options.keys.join(', ')}") unless options.empty?
   end
 
   def checksum_type
diff --git a/spec/unit/file_bucket/file_spec.rb b/spec/unit/file_bucket/file_spec.rb
index c4444ae77..ebf02438c 100755
--- a/spec/unit/file_bucket/file_spec.rb
+++ b/spec/unit/file_bucket/file_spec.rb
@@ -26,11 +26,17 @@ describe Puppet::FileBucket::File do
 
   it "should raise an error if changing content" do
     x = Puppet::FileBucket::File.new("first")
-    proc { x.contents = "new" }.should raise_error
+    expect { x.contents = "new" }.to raise_error(NoMethodError, /undefined method .contents=/)
   end
 
   it "should require contents to be a string" do
-    proc { Puppet::FileBucket::File.new(5) }.should raise_error(ArgumentError)
+    expect { Puppet::FileBucket::File.new(5) }.to raise_error(ArgumentError, /contents must be a String, got a Fixnum$/)
+  end
+
+  it "should complain about options other than :bucket_path" do
+    expect {
+      Puppet::FileBucket::File.new('5', :crazy_option => 'should not be passed')
+    }.to raise_error(ArgumentError, /Unknown option\(s\): crazy_option/)
   end
 
   it "should set the contents appropriately" do
@@ -61,7 +67,7 @@ describe Puppet::FileBucket::File do
 
   it "should reject a url-ish name with an invalid checksum" do
     bucket = Puppet::FileBucket::File.new(@contents)
-    lambda { bucket.name = "sha1/4a8ec4fa5f01b4ab1a0ab8cbccb709f0/new/path" }.should raise_error
+    expect { bucket.name = "sha1/4a8ec4fa5f01b4ab1a0ab8cbccb709f0/new/path" }.to raise_error(NoMethodError, /undefined method .name=/)
   end
 
   it "should convert the contents to PSON" do