Refactor en route to #5027 -- remove usestring parameter from lookupvar

The usestring parameter to lookupvar was objectionable for several reasons;
first, it performed a function orthogonal to the main purpose of the method,
second its default was the least common value, and third it was causing other
code to work for reasons that were not obvious (extlookup).

This refactor breaks the value-transforming function out into a seperate
method which allows the user to specify the value to be used in lieu of :undef
and removes the parameter.  The function, Scope#undef_as(default,exp) is
written so that it can be used in user code (templates, functions, etc.) if
needed.

This refactor will introduce a user-visible behaviour change in the case where
users were counting on lookupvar to return "" for undefined variables.  The
best solution is to have them use undef_as, replacing:

    lookupvar('myvar')

with

    undef_as('',lookupvar('myvar'))

(with the option to specify another default value if desired).  If this is too
objectionable, we could rename the existing lookupvar as raw_lookupvar and
define

    def lookupvar(v)
      undef_as('',raw_lookupvar(v))
    end

to restore the present behaviour.

5 files changed, 28 insertions, 36 deletions

diff --git a/spec/unit/parser/ast/arithmetic_operator_spec.rb b/spec/unit/parser/ast/arithmetic_operator_spec.rb
index 6f57fd9b6..0aab6f080 100755
--- a/spec/unit/parser/ast/arithmetic_operator_spec.rb
+++ b/spec/unit/parser/ast/arithmetic_operator_spec.rb
@@ -61,8 +61,8 @@ describe Puppet::Parser::AST::ArithmeticOperator do
   end
 
   it "should work for variables too" do
-    @scope.expects(:lookupvar).with("one", false).returns(1)
-    @scope.expects(:lookupvar).with("two", false).returns(2)
+    @scope.expects(:lookupvar).with("one").returns(1)
+    @scope.expects(:lookupvar).with("two").returns(2)
     one = ast::Variable.new( :value => "one" )
     two = ast::Variable.new( :value => "two" )
 
diff --git a/spec/unit/parser/ast/comparison_operator_spec.rb b/spec/unit/parser/ast/comparison_operator_spec.rb
index 169d92d0a..3efe28bb6 100755
--- a/spec/unit/parser/ast/comparison_operator_spec.rb
+++ b/spec/unit/parser/ast/comparison_operator_spec.rb
@@ -95,8 +95,8 @@ describe Puppet::Parser::AST::ComparisonOperator do
     one = Puppet::Parser::AST::Variable.new( :value => "one" )
     two = Puppet::Parser::AST::Variable.new( :value => "two" )
 
-    @scope.expects(:lookupvar).with("one", false).returns(1)
-    @scope.expects(:lookupvar).with("two", false).returns(2)
+    @scope.expects(:lookupvar).with("one").returns(1)
+    @scope.expects(:lookupvar).with("two").returns(2)
 
     operator = Puppet::Parser::AST::ComparisonOperator.new :lval => one, :operator => "<", :rval => two
     operator.evaluate(@scope).should == true
diff --git a/spec/unit/parser/ast/leaf_spec.rb b/spec/unit/parser/ast/leaf_spec.rb
index 3b8c14efa..97c996b40 100755
--- a/spec/unit/parser/ast/leaf_spec.rb
+++ b/spec/unit/parser/ast/leaf_spec.rb
@@ -336,12 +336,12 @@ describe Puppet::Parser::AST::Variable do
   end
 
   it "should lookup the variable in scope" do
-    @scope.expects(:lookupvar).with("myvar", false).returns(:myvalue)
+    @scope.expects(:lookupvar).with("myvar").returns(:myvalue)
     @var.safeevaluate(@scope).should == :myvalue
   end
 
   it "should return undef if the variable wasn't set" do
-    @scope.expects(:lookupvar).with("myvar", false).returns(:undefined)
+    @scope.expects(:lookupvar).with("myvar").returns(:undefined)
     @var.safeevaluate(@scope).should == :undef
   end
 
diff --git a/spec/unit/parser/scope_spec.rb b/spec/unit/parser/scope_spec.rb
index 44e255956..b37cb4add 100755
--- a/spec/unit/parser/scope_spec.rb
+++ b/spec/unit/parser/scope_spec.rb
@@ -76,16 +76,8 @@ describe Puppet::Parser::Scope do
   end
 
   describe "when looking up a variable" do
-    it "should default to an empty string" do
-      @scope.lookupvar("var").should == ""
-    end
-
-    it "should return an string when asked for a string" do
-      @scope.lookupvar("var", true).should == ""
-    end
-
-    it "should return ':undefined' for unset variables when asked not to return a string" do
-      @scope.lookupvar("var", false).should == :undefined
+    it "should return ':undefined' for unset variables" do
+      @scope.lookupvar("var").should == :undefined
     end
 
     it "should be able to look up values" do
@@ -151,32 +143,32 @@ describe Puppet::Parser::Scope do
         @scope.lookupvar("other::deep::klass::var").should == "otherval"
       end
 
-      it "should return an empty string for qualified variables that cannot be found in other classes" do
+      it "should return ':undefined' for qualified variables that cannot be found in other classes" do
         other_scope = create_class_scope("other::deep::klass")
 
-        @scope.lookupvar("other::deep::klass::var").should == ""
+        @scope.lookupvar("other::deep::klass::var").should == :undefined
       end
 
-      it "should warn and return an empty string for qualified variables whose classes have not been evaluated" do
+      it "should warn and return ':undefined' for qualified variables whose classes have not been evaluated" do
         klass = newclass("other::deep::klass")
         @scope.expects(:warning)
-        @scope.lookupvar("other::deep::klass::var").should == ""
+        @scope.lookupvar("other::deep::klass::var").should == :undefined
       end
 
-      it "should warn and return an empty string for qualified variables whose classes do not exist" do
+      it "should warn and return ':undefined' for qualified variables whose classes do not exist" do
         @scope.expects(:warning)
-        @scope.lookupvar("other::deep::klass::var").should == ""
+        @scope.lookupvar("other::deep::klass::var").should == :undefined
       end
 
       it "should return ':undefined' when asked for a non-string qualified variable from a class that does not exist" do
         @scope.stubs(:warning)
-        @scope.lookupvar("other::deep::klass::var", false).should == :undefined
+        @scope.lookupvar("other::deep::klass::var").should == :undefined
       end
 
       it "should return ':undefined' when asked for a non-string qualified variable from a class that has not been evaluated" do
         @scope.stubs(:warning)
         klass = newclass("other::deep::klass")
-        @scope.lookupvar("other::deep::klass::var", false).should == :undefined
+        @scope.lookupvar("other::deep::klass::var").should == :undefined
       end
     end
   end
@@ -291,7 +283,7 @@ describe Puppet::Parser::Scope do
 
       @scope.unset_ephemeral_var
 
-      @scope.lookupvar("1", false).should == :undefined
+      @scope.lookupvar("1").should == :undefined
     end
 
     it "should not remove classic variables when unset_ephemeral_var is called" do
@@ -301,7 +293,7 @@ describe Puppet::Parser::Scope do
 
       @scope.unset_ephemeral_var
 
-      @scope.lookupvar("myvar", false).should == :value1
+      @scope.lookupvar("myvar").should == :value1
     end
 
     it "should raise an error when setting it again" do
@@ -322,7 +314,7 @@ describe Puppet::Parser::Scope do
         @scope.setvar("0", :earliest, :ephemeral => true)
         @scope.new_ephemeral
         @scope.setvar("0", :latest, :ephemeral => true)
-        @scope.lookupvar("0", false).should == :latest
+        @scope.lookupvar("0").should == :latest
       end
 
       it "should be able to report the current level" do
@@ -353,7 +345,7 @@ describe Puppet::Parser::Scope do
         @scope.setvar("1", :value1, :ephemeral => true)
         @scope.new_ephemeral
         @scope.setvar("0", :value2, :ephemeral => true)
-        @scope.lookupvar("1", false).should == :value1
+        @scope.lookupvar("1").should == :value1
       end
 
       describe "when calling unset_ephemeral_var without a level" do
@@ -364,7 +356,7 @@ describe Puppet::Parser::Scope do
 
           @scope.unset_ephemeral_var
 
-          @scope.lookupvar("1", false).should == :undefined
+          @scope.lookupvar("1").should == :undefined
         end
       end
 
@@ -378,7 +370,7 @@ describe Puppet::Parser::Scope do
 
           @scope.unset_ephemeral_var(2)
 
-          @scope.lookupvar("1", false).should == :value2
+          @scope.lookupvar("1").should == :value2
         end
       end
     end
@@ -563,13 +555,13 @@ describe Puppet::Parser::Scope do
     it "should be able to unset normal variables" do
       @scope.setvar("foo", "bar")
       @scope.unsetvar("foo")
-      @scope.lookupvar("foo").should == ""
+      @scope.lookupvar("foo").should == :undefined
     end
 
     it "should be able to unset ephemeral variables" do
       @scope.setvar("0", "bar", :ephemeral => true)
       @scope.unsetvar("0")
-      @scope.lookupvar("0").should == ""
+      @scope.lookupvar("0").should == :undefined
     end
 
     it "should not unset ephemeral variables in previous ephemeral scope" do
diff --git a/spec/unit/parser/templatewrapper_spec.rb b/spec/unit/parser/templatewrapper_spec.rb
index afb0032fd..4a713b8a8 100755
--- a/spec/unit/parser/templatewrapper_spec.rb
+++ b/spec/unit/parser/templatewrapper_spec.rb
@@ -70,25 +70,25 @@ describe Puppet::Parser::TemplateWrapper do
   end
 
   it "should return the contents of a variable if called via method_missing" do
-    @scope.expects(:lookupvar).with("chicken", false).returns("is good")
+    @scope.expects(:lookupvar).with("chicken").returns("is good")
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     tw.chicken.should eql("is good")
   end
 
   it "should throw an exception if a variable is called via method_missing and it does not exist" do
-    @scope.expects(:lookupvar).with("chicken", false).returns(:undefined)
+    @scope.expects(:lookupvar).with("chicken").returns(:undefined)
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     lambda { tw.chicken }.should raise_error(Puppet::ParseError)
   end
 
   it "should allow you to check whether a variable is defined with has_variable?" do
-    @scope.expects(:lookupvar).with("chicken", false).returns("is good")
+    @scope.expects(:lookupvar).with("chicken").returns("is good")
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     tw.has_variable?("chicken").should eql(true)
   end
 
   it "should allow you to check whether a variable is not defined with has_variable?" do
-    @scope.expects(:lookupvar).with("chicken", false).returns(:undefined)
+    @scope.expects(:lookupvar).with("chicken").returns(:undefined)
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     tw.has_variable?("chicken").should eql(false)
   end