On Jul 5, 2011, at 2:12 PM, R.I.Pienaar wrote:
>
>
> ----- Original Message -----
>> On Jul 5, 2011, at 1:37 PM, Brice Figureau wrote:
>>
>>> On 05/07/11 21:36, Luke Kanies wrote:
>>>> This is primarily two changes:
>>>>
>>>> * Add [] and []= to Puppet::Parser::Scope
>>>> * Replace lookupvar/setvar usage with those two methods
>>>>
>>>> The first was pretty simple, but the second ended up touching a
>>>> lot of files.
>>>
>>> What's the underlying purpose (ie the big plan behind) of those
>>> changes?
>>
>> Hiera (Arri's extlookup replacement) needs Scope to have hash-like
>> behavior, and this is to provide that.
>>
>> Once I added the methods, I realized how ugly the existing methods
>> were, and it made sense to clean the whole thing up from there.
>>
>> Sorry, that should have been in my commit - I'll fix that.
>
> I think hiera aside this is a great improvement, it behaves more ruby like
> and less surprising, scope will just work how people who know ruby would think
> it would work and thats a big win.
>
> If in a template using foo would return nil if its unset that would be a
> big win too, means has_variable?("foo") can go away, not sure how related that
> is to your refactor though.
It's not necessarily related, but since I've got the whole thing swapped in...
Since we've got the 'include?' method now, and returning :undefined is not very
ruby-like, I ran through and made some further changes in behavior. Now the []
method returns nil for unknown values (instead of :undefined), and anything
that was previously testing for :undefined now tests scope.include?(var).
This provides the same behavior with much more ruby-like semantics and minimal
(but non-zero) incompatibility.
Here's the patch, and I've pushed it to the end of the series I published
earlier:
commit fee303d7dfdcc2381eb8b2fc4a33a2924461c561
Author: Luke Kanies <[email protected]>
Date: Tue Jul 5 16:32:03 2011 -0700
Scope[] now returns nil for undefined variables
Any code that previously checked whether lookupvar
(or the new []) returned :undefined should now check
whether 'scope.include?(var)'. Thus, you can have
the same behavior, but it takes a bit different code
to get it.
Signed-off-by: Luke Kanies <[email protected]>
diff --git a/lib/puppet/parser/ast/leaf.rb b/lib/puppet/parser/ast/leaf.rb
index 64a1974..3efb52f 100644
--- a/lib/puppet/parser/ast/leaf.rb
+++ b/lib/puppet/parser/ast/leaf.rb
@@ -124,10 +124,11 @@ class Puppet::Parser::AST
# not include syntactical constructs, like '$' and '{}').
def evaluate(scope)
parsewrap do
- if (var = scope[@value, {:file => file, :line => line}]) == :undefined
- var = :undef
+ if ! scope.include?(@value)
+ :undef
+ else
+ scope[@value, {:file => file, :line => line}]
end
- var
end
end
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 6b072bd..56887c3 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -258,7 +258,8 @@ class Puppet::Parser::Resource < Puppet::Resource
def add_backward_compatible_relationship_param(name)
# Skip metaparams for which we get no value.
- return unless val = scope[name.to_s] and val != :undefined
+ return unless scope.include?(name.to_s)
+ val = scope[name.to_s]
# The default case: just set the value
set_parameter(name, val) and return unless @parameters[name]
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 3a87f96..9d84c7e 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -57,7 +57,7 @@ class Puppet::Parser::Scope
rescue RuntimeError => e
location = (options[:file] && options[:line]) ? " at
#{options[:file]}:#{options[:line]}" : ''
warning "Could not look up qualified variable '#{name}';
#{e.message}#{location}"
- :undefined
+ nil
end
elsif ephemeral_include?(name) or table.include?(name)
# We can't use "if table[name]" here because the value might be false
@@ -69,7 +69,7 @@ class Puppet::Parser::Scope
elsif parent
parent[name,options.merge(:dynamic => (dynamic || options[:dynamic]))]
else
- :undefined
+ nil
end
end
@@ -92,7 +92,7 @@ class Puppet::Parser::Scope
end
def include?(name)
- self[name] != :undefined
+ ! self[name].nil?
end
# Is the value true? This allows us to control the definition of truth
@@ -244,7 +244,11 @@ class Puppet::Parser::Scope
end
def undef_as(x,v)
- (v == :undefined) ? x : (v == :undef) ? x : v
+ if v.nil? or v == :undef
+ x
+ else
+ v
+ end
end
def qualified_scope(classname)
diff --git a/lib/puppet/parser/templatewrapper.rb
b/lib/puppet/parser/templatewrapper.rb
index 83d18cc..9336e70 100644
--- a/lib/puppet/parser/templatewrapper.rb
+++ b/lib/puppet/parser/templatewrapper.rb
@@ -25,7 +25,7 @@ class Puppet::Parser::TemplateWrapper
# Should return true if a variable is defined, false if it is not
def has_variable?(name)
- scope[name.to_s, {:file => file, :line => script_line}] != :undefined
+ scope.include?(name.to_s)
end
# Allow templates to access the defined classes
@@ -56,9 +56,8 @@ class Puppet::Parser::TemplateWrapper
# the missing_method definition here until we declare the syntax finally
# dead.
def method_missing(name, *args)
- value = scope[name.to_s, {:file => file,:line => script_line}]
- if value != :undefined
- return value
+ if scope.include?(name.to_s)
+ return scope[name.to_s, {:file => file,:line => script_line}]
else
# Just throw an error immediately, instead of searching for
# other missingmethod things or whatever.
diff --git a/spec/unit/parser/scope_spec.rb b/spec/unit/parser/scope_spec.rb
index d3ea8df..3c0d15c 100755
--- a/spec/unit/parser/scope_spec.rb
+++ b/spec/unit/parser/scope_spec.rb
@@ -88,8 +88,8 @@ describe Puppet::Parser::Scope do
@scope.lookupvar("var").should == "yep"
end
- it "should return ':undefined' for unset variables" do
- @scope["var"].should == :undefined
+ it "should return nil for unset variables" do
+ @scope["var"].should be_nil
end
it "should be able to look up values" do
@@ -180,32 +180,32 @@ describe Puppet::Parser::Scope do
@scope["other::deep::klass::var"].should == "otherval"
end
- it "should return ':undefined' for qualified variables that cannot be
found in other classes" do
+ it "should return nil for qualified variables that cannot be found in
other classes" do
other_scope = create_class_scope("other::deep::klass")
- @scope["other::deep::klass::var"].should == :undefined
+ @scope["other::deep::klass::var"].should be_nil
end
- it "should warn and return ':undefined' for qualified variables whose
classes have not been evaluated" do
+ it "should warn and return nil for qualified variables whose classes
have not been evaluated" do
klass = newclass("other::deep::klass")
@scope.expects(:warning)
- @scope["other::deep::klass::var"].should == :undefined
+ @scope["other::deep::klass::var"].should be_nil
end
- it "should warn and return ':undefined' for qualified variables whose
classes do not exist" do
+ it "should warn and return nil for qualified variables whose classes do
not exist" do
@scope.expects(:warning)
- @scope["other::deep::klass::var"].should == :undefined
+ @scope["other::deep::klass::var"].should be_nil
end
- it "should return ':undefined' when asked for a non-string qualified
variable from a class that does not exist" do
+ it "should return nil when asked for a non-string qualified variable
from a class that does not exist" do
@scope.stubs(:warning)
- @scope["other::deep::klass::var"].should == :undefined
+ @scope["other::deep::klass::var"].should be_nil
end
- it "should return ':undefined' when asked for a non-string qualified
variable from a class that has not been evaluated" do
+ it "should return nil 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["other::deep::klass::var"].should == :undefined
+ @scope["other::deep::klass::var"].should be_nil
end
end
end
@@ -320,7 +320,7 @@ describe Puppet::Parser::Scope do
@scope.unset_ephemeral_var
- @scope["1"].should == :undefined
+ @scope["1"].should be_nil
end
it "should not remove classic variables when unset_ephemeral_var is
called" do
@@ -393,7 +393,7 @@ describe Puppet::Parser::Scope do
@scope.unset_ephemeral_var
- @scope["1"].should == :undefined
+ @scope["1"].should be_nil
end
end
@@ -449,13 +449,13 @@ describe Puppet::Parser::Scope do
it "should be able to unset normal variables" do
@scope["foo"] = "bar"
@scope.unsetvar("foo")
- @scope["foo"].should == :undefined
+ @scope["foo"].should be_nil
end
it "should be able to unset ephemeral variables" do
@scope.setvar("0", "bar", :ephemeral => true)
@scope.unsetvar("0")
- @scope["0"].should == :undefined
+ @scope["0"].should be_nil
end
it "should not unset ephemeral variables in previous ephemeral scope" do
diff --git a/test/language/scope.rb b/test/language/scope.rb
index 57824d8..c80178e 100755
--- a/test/language/scope.rb
+++ b/test/language/scope.rb
@@ -47,7 +47,7 @@ class TestScope < Test::Unit::TestCase
# Now set a var in the midscope and make sure the mid and bottom can see
it but not the top
midscope['second'] = "midval"
- assert_equal(:undefined, scopes[:top]['second'], "Found child var in top
scope")
+ assert_nil(scopes[:top]['second'], "Found child var in top scope")
[:mid, :bot].each do |name|
assert_equal("midval", scopes[name]['second'], "Could not find var in
#{name}")
end
@@ -55,7 +55,7 @@ class TestScope < Test::Unit::TestCase
# And set something in the bottom, and make sure we only find it there.
botscope['third'] = "botval"
[:top, :mid].each do |name|
- assert_equal(:undefined, scopes[name]['third'], "Found child var in top
scope")
+ assert_nil(scopes[name]['third'], "Found child var in top scope")
end
assert_equal("botval", scopes[:bot]['third'], "Could not find var in
bottom scope")
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
