A small comment, not worth sending another patch. Otherwise, +1
On Jul 26, 2009, at 9:03 AM, Brice Figureau wrote: > > This patch uses the unused AST::HostName as the only way to reference > a node in the AST nodes array. > The AST::HostName respect the hash properties of the underlying > string, to keep the O(1) hash properties. > > Signed-off-by: Brice Figureau <[email protected]> > --- > lib/puppet/parser/ast/leaf.rb | 16 +- > lib/puppet/parser/ast/node.rb | 5 - > lib/puppet/parser/grammar.ra | 10 +- > lib/puppet/parser/loaded_code.rb | 9 +- > lib/puppet/parser/parser.rb | 1129 +++++++++++++ > +--------------- > lib/puppet/parser/parser_support.rb | 4 +- > spec/unit/parser/ast/leaf.rb | 45 ++- > spec/unit/parser/loaded_code.rb | 73 ++- > spec/unit/parser/parser.rb | 90 +++- > test/lib/puppettest/support/collection.rb | 2 +- > 10 files changed, 786 insertions(+), 597 deletions(-) > > diff --git a/lib/puppet/parser/ast/leaf.rb b/lib/puppet/parser/ast/ > leaf.rb > index de5876e..866d2b6 100644 > --- a/lib/puppet/parser/ast/leaf.rb > +++ b/lib/puppet/parser/ast/leaf.rb > @@ -90,11 +90,25 @@ class Puppet::Parser::AST > def initialize(hash) > super > > - unless @value =~ %r{^[0-9a-zA-Z\-]+(\.[0-9a-zA-Z\-]+)*$} > + @value = @value.to_s.downcase > + if @value =~ /[^-\w.]/ > raise Puppet::DevError, > "'%s' is not a valid hostname" % @value > end > end > + > + def to_classname > + return @value > + end > + > + def eql?(value) > + value = value.value if value.is_a?(HostName) > + return @value.eql?(value) > + end > + > + def hash > + return @value.hash > + end > end > > # A simple variable. This object is only used during > interpolation; > diff --git a/lib/puppet/parser/ast/node.rb b/lib/puppet/parser/ast/ > node.rb > index faa7fa1..b2d4044 100644 > --- a/lib/puppet/parser/ast/node.rb > +++ b/lib/puppet/parser/ast/node.rb > @@ -11,11 +11,6 @@ class Puppet::Parser::AST::Node < > Puppet::Parser::AST::HostClass > def initialize(options) > @parentclass = nil > super > - > - # Do some validation on the node name > - if @name =~ /[^-\w.]/ > - raise Puppet::ParseError, "Invalid node name %s" % @name > - end > end > > def namespace > diff --git a/lib/puppet/parser/grammar.ra b/lib/puppet/parser/ > grammar.ra > index 072f351..ed55d21 100644 > --- a/lib/puppet/parser/grammar.ra > +++ b/lib/puppet/parser/grammar.ra > @@ -684,11 +684,15 @@ classname: NAME { result = val[0] > [:value] } > > # Multiple hostnames, as used for node names. These are all literal > # strings, not AST objects. > -hostnames: hostname > - | hostnames COMMA hostname { > +hostnames: nodename > + | hostnames COMMA nodename { > result = val[0] > result = [result] unless result.is_a?(Array) > - result << val[2][:value] > + result << val[2] > +} > + > +nodename: hostname { > + result = ast AST::HostName, :value => val[0] > } > > hostname: NAME { result = val[0][:value] } > diff --git a/lib/puppet/parser/loaded_code.rb b/lib/puppet/parser/ > loaded_code.rb > index 6c51961..4bb03ee 100644 > --- a/lib/puppet/parser/loaded_code.rb > +++ b/lib/puppet/parser/loaded_code.rb > @@ -14,11 +14,11 @@ class Puppet::Parser::LoadedCode > end > > def add_node(name, code) > - @nodes[munge_name(name)] = code > + @nodes[check_name(name)] = code > end > > def node(name) > - @nodes[munge_name(name)] > + @nodes[check_name(name)] > end > > def nodes? > @@ -88,4 +88,9 @@ class Puppet::Parser::LoadedCode > def munge_name(name) > name.to_s.downcase > end > + > + def check_name(name) > + name = Puppet::Parser::AST::HostName.new(:value => name) > unless name.is_a?(Puppet::Parser::AST::HostName) > + name > + end This method could use just a bit of a comment to explain why it's necessary, especially given how magical the 'hash' method is. > > end > diff --git a/lib/puppet/parser/parser.rb b/lib/puppet/parser/parser.rb > index cedc7de..dbe0be4 100644 > --- a/lib/puppet/parser/parser.rb > +++ b/lib/puppet/parser/parser.rb > <Patch Elided> > diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/ > parser_support.rb > index e1af2fe..b13fbb4 100644 > --- a/lib/puppet/parser/parser_support.rb > +++ b/lib/puppet/parser/parser_support.rb > @@ -354,7 +354,7 @@ class Puppet::Parser::Parser > names = [names] unless names.instance_of?(Array) > doc = lexer.getcomment > names.collect do |name| > - name = name.to_s.downcase > + name = AST::HostName.new :value => name unless > name.is_a?(AST::HostName) > if other = @loaded_code.node(name) > error("Node %s is already defined at %s:%s; cannot > redefine" % [other.name, other.file, other.line]) > end > @@ -372,7 +372,7 @@ class Puppet::Parser::Parser > args[:parentclass] = options[:parent] > end > node = ast(AST::Node, args) > - node.classname = name > + node.classname = name.to_classname > @loaded_code.add_node(name, node) > node > end > diff --git a/spec/unit/parser/ast/leaf.rb b/spec/unit/parser/ast/ > leaf.rb > index 8315b80..38b753b 100755 > --- a/spec/unit/parser/ast/leaf.rb > +++ b/spec/unit/parser/ast/leaf.rb > @@ -136,7 +136,9 @@ end > describe Puppet::Parser::AST::HostName do > before :each do > @scope = stub 'scope' > - @value = stub 'value', :is_a? => true, :=~ => true > + @value = stub 'value', :=~ => false > + @value.stubs(:to_s).returns(@value) > + @value.stubs(:downcase).returns(@value) > @host = Puppet::Parser::AST::HostName.new( :value => @value) > end > > @@ -144,7 +146,48 @@ describe Puppet::Parser::AST::HostName do > lambda { Puppet::Parser::AST::HostName.new( :value => "not > an hostname!" ) }.should raise_error > end > > + it "should stringify the value" do > + value = stub 'value', :=~ => false > + > + value.expects(:to_s).returns("test") > + > + Puppet::Parser::AST::HostName.new(:value => value) > + end > + > + it "should downcase the value" do > + value = stub 'value', :=~ => false > + value.stubs(:to_s).returns("UPCASED") > + host = Puppet::Parser::AST::HostName.new(:value => value) > + > + host.value == "upcased" > + end > + > it "should evaluate to its value" do > @host.evaluate(@scope).should == @value > end > + > + it "should implement to_classname" do > + @host.should respond_to(:to_classname) > + end > + > + it "should return the downcased nodename as classname" do > + host = Puppet::Parser::AST::HostName.new( :value => > "KLASSNAME" ) > + host.to_classname.should == "klassname" > + end > + > + it "should delegate eql? to the underlying value if it is an > HostName" do > + @value.expects(:eql?).with("value") > + @host.eql?("value") > + end > + > + it "should delegate eql? to the underlying value if it is not > an HostName" do > + value = stub 'compared', :is_a? => true, :value => "value" > + @value.expects(:eql?).with("value") > + @host.eql?(value) > + end > + > + it "should delegate hash to the underlying value" do > + @value.expects(:hash) > + @host.hash > + end > end > diff --git a/spec/unit/parser/loaded_code.rb b/spec/unit/parser/ > loaded_code.rb > index d2986bf..d33bda9 100644 > --- a/spec/unit/parser/loaded_code.rb > +++ b/spec/unit/parser/loaded_code.rb > @@ -37,49 +37,49 @@ describe Puppet::Parser::LoadedCode do > describe "when finding a qualified instance" do > it "should return any found instance if the instance name is > fully qualified" do > loader = Puppet::Parser::LoadedCode.new > - loader.add_node "foo::bar", "yay" > - loader.find("namespace", "::foo::bar", :node).should == > "yay" > + loader.add_hostclass "foo::bar", "yay" > + loader.find("namespace", > "::foo::bar", :hostclass).should == "yay" > end > > it "should return nil if the instance name is fully > qualified and no such instance exists" do > loader = Puppet::Parser::LoadedCode.new > - loader.find("namespace", "::foo::bar", :node).should > be_nil > + loader.find("namespace", > "::foo::bar", :hostclass).should be_nil > end > > it "should return the partially qualified object if it > exists in the provided namespace" do > loader = Puppet::Parser::LoadedCode.new > - loader.add_node "foo::bar::baz", "yay" > - loader.find("foo", "bar::baz", :node).should == "yay" > + loader.add_hostclass "foo::bar::baz", "yay" > + loader.find("foo", "bar::baz", :hostclass).should == > "yay" > end > > it "should return the unqualified object if it exists in the > provided namespace" do > loader = Puppet::Parser::LoadedCode.new > - loader.add_node "foo::bar", "yay" > - loader.find("foo", "bar", :node).should == "yay" > + loader.add_hostclass "foo::bar", "yay" > + loader.find("foo", "bar", :hostclass).should == "yay" > end > > it "should return the unqualified object if it exists in the > parent namespace" do > loader = Puppet::Parser::LoadedCode.new > - loader.add_node "foo::bar", "yay" > - loader.find("foo::bar::baz", "bar", :node).should == > "yay" > + loader.add_hostclass "foo::bar", "yay" > + loader.find("foo::bar::baz", "bar", :hostclass).should > == "yay" > end > > it "should should return the partially qualified object if > it exists in the parent namespace" do > loader = Puppet::Parser::LoadedCode.new > - loader.add_node "foo::bar::baz", "yay" > - loader.find("foo::bar", "bar::baz", :node).should == > "yay" > + loader.add_hostclass "foo::bar::baz", "yay" > + loader.find("foo::bar", "bar::baz", :hostclass).should > == "yay" > end > > it "should return the qualified object if it exists in the > root namespace" do > loader = Puppet::Parser::LoadedCode.new > - loader.add_node "foo::bar::baz", "yay" > - loader.find("foo::bar", "foo::bar::baz", :node).should > == "yay" > + loader.add_hostclass "foo::bar::baz", "yay" > + loader.find("foo::bar", > "foo::bar::baz", :hostclass).should == "yay" > end > > it "should return nil if the object cannot be found" do > loader = Puppet::Parser::LoadedCode.new > - loader.add_node "foo::bar::baz", "yay" > - loader.find("foo::bar", "eh", :node).should be_nil > + loader.add_hostclass "foo::bar::baz", "yay" > + loader.find("foo::bar", "eh", :hostclass).should be_nil > end > end > > @@ -110,4 +110,47 @@ describe Puppet::Parser::LoadedCode do > it "should indicate whether no nodes are defined" do > Puppet::Parser::LoadedCode.new.should_not be_nodes > end > + > + describe "when adding nodes" do > + it "should create an HostName if nodename is a string" do > + Puppet::Parser::AST::HostName.expects(:new).with(:value > => "foo") > + loader = Puppet::Parser::LoadedCode.new > + loader.add_node("foo", "bar") > + end > + > + it "should not create an HostName if nodename is an > HostName" do > + name = Puppet::Parser::AST::HostName.new(:value => "foo") > + > + Puppet::Parser::AST::HostName.expects(:new).with(:value > => "foo").never > + > + loader = Puppet::Parser::LoadedCode.new > + loader.add_node(name, "bar") > + end > + end > + > + describe "when finding nodes" do > + it "should create an HostName if nodename is a string" do > + Puppet::Parser::AST::HostName.expects(:new).with(:value > => "foo") > + loader = Puppet::Parser::LoadedCode.new > + loader.node("foo") > + end > + > + it "should not create an HostName if nodename is an > HostName" do > + name = Puppet::Parser::AST::HostName.new(:value => "foo") > + > + Puppet::Parser::AST::HostName.expects(:new).with(:value > => "foo").never > + > + loader = Puppet::Parser::LoadedCode.new > + loader.node(name) > + end > + > + it "should be able to find nobe by HostName" do > + namein = Puppet::Parser::AST::HostName.new(:value => > "foo") > + nameout = Puppet::Parser::AST::HostName.new(:value => > "foo") > + loader = Puppet::Parser::LoadedCode.new > + > + loader.add_node(namein, "bar") > + loader.node(nameout) == "bar" > + end > + end > end > diff --git a/spec/unit/parser/parser.rb b/spec/unit/parser/parser.rb > index 44e4bed..46a5829 100755 > --- a/spec/unit/parser/parser.rb > +++ b/spec/unit/parser/parser.rb > @@ -7,7 +7,8 @@ describe Puppet::Parser do > ast = Puppet::Parser::AST > > before :each do > - @parser = Puppet::Parser::Parser.new :environment => > "development" > + @loaded_code = Puppet::Parser::LoadedCode.new > + @parser = Puppet::Parser::Parser.new :environment => > "development", :loaded_code => @loaded_code > @true_ast = Puppet::Parser::AST::Boolean.new :value => true > end > > @@ -225,4 +226,91 @@ describe Puppet::Parser do > @parser.ast(Puppet::Parser::AST::Definition) > end > end > + > + describe "when creating a node" do > + before :each do > + @lexer = stub 'lexer' > + @lexer.stubs(:getcomment) > + @parser.stubs(:lexer).returns(@lexer) > + @node = stub_everything 'node' > + @parser.stubs(:ast).returns(@node) > + @parser.stubs(:node).returns(nil) > + > + @nodename = stub 'nodename', :is_a? => > false, :to_classname => "node" > + > @nodename > .stubs(:is_a?).with(Puppet::Parser::AST::HostName).returns(true) > + end > + > + it "should get the lexer stacked comments" do > + @lexer.expects(:getcomment) > + > + @parser.newnode(@nodename) > + end > + > + it "should create an HostName if needed" do > + Puppet::Parser::AST::HostName.expects(:new).with(:value > => "node").returns(@nodename) > + > + @parser.newnode("node") > + end > + > + it "should raise an error if the node already exists" do > + @loaded_code.stubs(:node).with(@nodename).returns(@node) > + > + lambda { @parser.newnode(@nodename) }.should raise_error > + end > + > + it "should store the created node in the loaded code" do > + @loaded_code.expects(:add_node).with(@nodename, @node) > + > + @parser.newnode(@nodename) > + end > + > + it "should create the node with code if provided" do > + @parser.stubs(:ast).with { |*args| args[1][:code] > == :code }.returns(@node) > + > + @parser.newnode(@nodename, :code => :code) > + end > + > + it "should create the node with a parentclass if provided" do > + @parser.stubs(:ast).with { |*args| args[1][:parent] > == :parent }.returns(@node) > + > + @parser.newnode(@nodename, :parent => :parent) > + end > + > + it "should set the node classname from the HostName" do > + @nodename.stubs(:to_classname).returns(:classname) > + > + @node.expects(:classname=).with(:classname) > + > + @parser.newnode(@nodename) > + end > + > + it "should return an array of nodes" do > + @parser.newnode(@nodename).should == [...@node] > + end > + end > + > + describe "when retrieving a specific node" do > + it "should delegate to the loaded_code node" do > + @loaded_code.expects(:node).with("node") > + > + @parser.node("node") > + end > + end > + > + describe "when retrieving a specific class" do > + it "should delegate to the loaded code" do > + @loaded_code.expects(:hostclass).with("class") > + > + @parser.hostclass("class") > + end > + end > + > + describe "when retrieving a specific definitions" do > + it "should delegate to the loaded code" do > + @loaded_code.expects(:definition).with("define") > + > + @parser.definition("define") > + end > + end > + > end > diff --git a/test/lib/puppettest/support/collection.rb b/test/lib/ > puppettest/support/collection.rb > index 43df030..9da15ea 100644 > --- a/test/lib/puppettest/support/collection.rb > +++ b/test/lib/puppettest/support/collection.rb > @@ -18,7 +18,7 @@ module PuppetTest::Support::Collection > query = nil > > assert_nothing_raised("Could not parse '#{str}'") do > - query = > parser.parse(code).classes[""].code[0].query > + query = > parser.parse(code).hostclass("").code[0].query > end > > yield str, res, query > -- > 1.6.0.2 > > > > -- You wake me up early in the morning to tell me I am right? Please wait until I am wrong. -- Johann von Neumann --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
