A couple of comments below; otherwise, looks great.
On Jul 26, 2009, at 9:03 AM, Brice Figureau wrote:
>
> The case and selector statements define ephemeral vars, like 'if'.
>
> Usage:
> $var = "foobar"
> case $var {
> "foo": {
> notify { "got a foo": }
> }
> /(.*)bar$/: {
> notify{ "hey we got a $1": }
> }
> }
>
> $val = $test ? {
> /^match.*$/ => "matched",
> default => "default"
> }
>
> Signed-off-by: Brice Figureau <[email protected]>
> ---
> lib/puppet/parser/ast/caseopt.rb | 10 +
> lib/puppet/parser/ast/casestatement.rb | 11 +-
> lib/puppet/parser/ast/selector.rb | 12 +-
> lib/puppet/parser/grammar.ra | 1 +
> lib/puppet/parser/parser.rb | 1179 ++++++++++++++++
> +---------------
> spec/unit/parser/ast/casestatement.rb | 143 ++++
> spec/unit/parser/ast/selector.rb | 156 +++++
> test/data/snippets/casestatement.pp | 7 +
> test/data/snippets/selectorvalues.pp | 7 +
> test/language/snippets.rb | 3 +-
> test/lib/puppettest/parsertesting.rb | 5 +
> 11 files changed, 962 insertions(+), 572 deletions(-)
> create mode 100755 spec/unit/parser/ast/casestatement.rb
> create mode 100755 spec/unit/parser/ast/selector.rb
>
> diff --git a/lib/puppet/parser/ast/caseopt.rb b/lib/puppet/parser/
> ast/caseopt.rb
> index 824bde8..47f32e2 100644
> --- a/lib/puppet/parser/ast/caseopt.rb
> +++ b/lib/puppet/parser/ast/caseopt.rb
> @@ -51,6 +51,16 @@ class Puppet::Parser::AST
> end
> end
>
> + def eachopt
> + if @value.is_a?(AST::ASTArray)
> + @value.each { |subval|
> + yield subval
> + }
> + else
> + yield @value
> + end
> + end
If I'm reading this correctly, this is used for the following style of
code:
case $foo {
[a, b]: { ... }
...
}
Right?
If so, I don't think it's necessary. That syntax already works
without the array, and this adds a bit of weirdness. E.g.:
case $foo {
a, [b, c]: {...}
}
In that situation, each value is yielded separately, rather than the
single value and then the array.
Or am I reading this incorrectly?
>
> # Evaluate the actual statements; this only gets called if
> # our option matched.
> def evaluate(scope)
> diff --git a/lib/puppet/parser/ast/casestatement.rb b/lib/puppet/
> parser/ast/casestatement.rb
> index 0727479..69d0f1b 100644
> --- a/lib/puppet/parser/ast/casestatement.rb
> +++ b/lib/puppet/parser/ast/casestatement.rb
> @@ -21,9 +21,8 @@ class Puppet::Parser::AST
> # Iterate across the options looking for a match.
> default = nil
> @options.each { |option|
> - option.eachvalue(scope) { |opval|
> - opval = opval.downcase if ! sensitive and
> opval.respond_to?(:downcase)
> - if opval == value
> + option.eachopt { |opt|
> + if opt.evaluate_match(value, scope, :file =>
> file, :line => line, :sensitive => sensitive)
> found = true
> break
> end
> @@ -31,7 +30,11 @@ class Puppet::Parser::AST
>
> if found
> # we found a matching option
> - retvalue = option.safeevaluate(scope)
> + begin
> + retvalue = option.safeevaluate(scope)
> + ensure
> + scope.unset_ephemeral_var
> + end
> break
> end
If you end up modifying this patch much, can you drop that 'if' with a
short-circuit return like 'return retvalue unless found'?
I expect most of the AST code could be shortened quite a bit like this.
>
> diff --git a/lib/puppet/parser/ast/selector.rb b/lib/puppet/parser/
> ast/selector.rb
> index ecad163..dceec61 100644
> --- a/lib/puppet/parser/ast/selector.rb
> +++ b/lib/puppet/parser/ast/selector.rb
> @@ -32,13 +32,13 @@ class Puppet::Parser::AST
>
> # Then look for a match in the options.
> @values.each { |obj|
> - param = obj.param.safeevaluate(scope)
> - if ! sensitive && param.respond_to?(:downcase)
> - param = param.downcase
> - end
> - if param == paramvalue
> + if obj.param.evaluate_match(paramvalue,
> scope, :file => file, :line => line, :sensitive => sensitive)
> # we found a matching option
> - retvalue = obj.value.safeevaluate(scope)
> + begin
> + retvalue = obj.value.safeevaluate(scope)
> + ensure
> + scope.unset_ephemeral_var
> + end
> found = true
> break
> elsif obj.param.is_a?(Default)
> diff --git a/lib/puppet/parser/grammar.ra b/lib/puppet/parser/
> grammar.ra
> index 2040ee8..072f351 100644
> --- a/lib/puppet/parser/grammar.ra
> +++ b/lib/puppet/parser/grammar.ra
> @@ -620,6 +620,7 @@ selectlhand: name
> | DEFAULT {
> result = ast AST::Default, :value => val[0][:value], :line =>
> val[0][:line]
> }
> + | regex
>
> # These are only used for importing, and we don't interpolate there.
> qtexts: quotedtext { result = [val[0].value] }
> diff --git a/lib/puppet/parser/parser.rb b/lib/puppet/parser/parser.rb
> index 906304b..cedc7de 100644
> --- a/lib/puppet/parser/parser.rb
> +++ b/lib/puppet/parser/parser.rb
> <Patch Elided>
> diff --git a/spec/unit/parser/ast/casestatement.rb b/spec/unit/
> parser/ast/casestatement.rb
> new file mode 100755
> index 0000000..554e295
> --- /dev/null
> +++ b/spec/unit/parser/ast/casestatement.rb
> @@ -0,0 +1,143 @@
> +#!/usr/bin/env ruby
> +
> +require File.dirname(__FILE__) + '/../../../spec_helper'
> +
> +describe Puppet::Parser::AST::CaseStatement do
> + before :each do
> + @scope = Puppet::Parser::Scope.new()
> + end
Very excited to have all of this new test code.
>
> + describe "when evaluating" do
> +
> + before :each do
> + @test = stub 'test'
> + @test.stubs(:safeevaluate).with(@scope).returns("value")
> +
> + @option1 = stub 'option1', :eachopt => nil, :default?
> => false
> + @option2 = stub 'option2', :eachopt => nil, :default?
> => false
> +
> + @options = stub 'options'
> + @options.stubs(:each).multiple_yields(@option1, @option2)
> +
> + @casestmt =
> Puppet::Parser::AST::CaseStatement.new :test => @test, :options =>
> @options
> + end
> +
> + it "should evaluate test" do
> + @test.expects(:safeevaluate).with(@scope)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should downcase the evaluated test value if allowed" do
> + Puppet.stubs(:[]).with(:casesensitive).returns(false)
> + value = stub 'test'
> + @test.stubs(:safeevaluate).with(@scope).returns(value)
> +
> + value.expects(:downcase)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should scan each option" do
> + @options.expects(:each).multiple_yields(@option1,
> @option2)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + describe "when scanning options" do
> + before :each do
> + @opval1 = stub_everything 'opval1'
> + @option1.stubs(:eachopt).yields(@opval1)
> +
> + @opval2 = stub_everything 'opval2'
> + @option2.stubs(:eachopt).yields(@opval2)
> + end
> +
> + it "should evaluate each sub-option" do
> + @option1.expects(:eachopt)
> + @option2.expects(:eachopt)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should evaluate first matching option" do
> + @opval2.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" }.returns(true)
> + @option2.expects(:safeevaluate).with(@scope)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should evaluate_match with sensitive parameter" do
> + Puppet.stubs(:[]).with(:casesensitive).returns(true)
> + @opval1.expects(:evaluate_match).with { |*arg|
> arg[2][:sensitive] == true }
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should return the first matching evaluated option" do
> + @opval2.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" }.returns(true)
> +
> @option2.stubs(:safeevaluate).with(@scope).returns(:result)
> +
> + @casestmt.evaluate(@scope).should == :result
> + end
> +
> + it "should evaluate the default option if none matched"
> do
> + @option1.stubs(:default?).returns(true)
> + @option1.expects(:safeevaluate).with(@scope)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should return the default evaluated option if none
> matched" do
> + @option1.stubs(:default?).returns(true)
> +
> @option1.stubs(:safeevaluate).with(@scope).returns(:result)
> +
> + @casestmt.evaluate(@scope).should == :result
> + end
> +
> + it "should return nil if nothing matched" do
> + @casestmt.evaluate(@scope).should be_nil
> + end
> +
> + it "should match and set scope ephemeral variables" do
> + @opval1.expects(:evaluate_match).with { |*arg|
> arg[0] == "value" and arg[1] == @scope }
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should evaluate this regex option if it matches" do
> + @opval1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> +
> + @option1.expects(:safeevaluate).with(@scope)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should return this evaluated regex option if it
> matches" do
> + @opval1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> +
> @option1.stubs(:safeevaluate).with(@scope).returns(:result)
> +
> + @casestmt.evaluate(@scope).should == :result
> + end
> +
> + it "should unset scope ephemeral variables after option
> evaluation" do
> + @opval1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> +
> @option1.stubs(:safeevaluate).with(@scope).returns(:result)
> +
> + @scope.expects(:unset_ephemeral_var)
> +
> + @casestmt.evaluate(@scope)
> + end
> +
> + it "should not leak ephemeral variables even if
> evaluation fails" do
> + @opval1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> + @option1.stubs(:safeevaluate).with(@scope).raises
> +
> + @scope.expects(:unset_ephemeral_var)
> +
> + lambda { @casestmt.evaluate(@scope) }.should
> raise_error
> + end
> + end
> +
> + end
> +end
> diff --git a/spec/unit/parser/ast/selector.rb b/spec/unit/parser/ast/
> selector.rb
> new file mode 100755
> index 0000000..8b00577
> --- /dev/null
> +++ b/spec/unit/parser/ast/selector.rb
> @@ -0,0 +1,156 @@
> +#!/usr/bin/env ruby
> +
> +require File.dirname(__FILE__) + '/../../../spec_helper'
> +
> +describe Puppet::Parser::AST::Selector do
> + before :each do
> + @scope = Puppet::Parser::Scope.new()
> + end
> +
> + describe "when evaluating" do
> +
> + before :each do
> + @param = stub 'param'
> + @param.stubs(:safeevaluate).with(@scope).returns("value")
> +
> + @value1 = stub 'value1'
> + @param1 = stub_everything 'param1'
> +
> @param1.stubs(:safeevaluate).with(@scope).returns(@param1)
> +
> @param1.stubs(:respond_to?).with(:downcase).returns(false)
> + @value1.stubs(:param).returns(@param1)
> + @value1.stubs(:value).returns(@value1)
> +
> + @value2 = stub 'value2'
> + @param2 = stub_everything 'param2'
> +
> @param2.stubs(:safeevaluate).with(@scope).returns(@param2)
> +
> @param2.stubs(:respond_to?).with(:downcase).returns(false)
> + @value2.stubs(:param).returns(@param2)
> + @value2.stubs(:value).returns(@value2)
> +
> + @values = stub 'values', :instance_of? => true
> + @values.stubs(:each).multiple_yields(@value1, @value2)
> +
> + @selector = Puppet::Parser::AST::Selector.new :param =>
> @param, :values => @values
> + @selector.stubs(:fail)
> + end
> +
> + it "should evaluate param" do
> + @param.expects(:safeevaluate).with(@scope)
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should downcase the evaluated param value if allowed" do
> + Puppet.stubs(:[]).with(:casesensitive).returns(false)
> + value = stub 'param'
> + @param.stubs(:safeevaluate).with(@scope).returns(value)
> +
> + value.expects(:downcase)
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should scan each option" do
> + @values.expects(:each).multiple_yields(@value1, @value2)
> +
> + @selector.evaluate(@scope)
> + end
> +
> + describe "when scanning values" do
> + it "should evaluate first matching option" do
> + @param2.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" }.returns(true)
> + @value2.expects(:safeevaluate).with(@scope)
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should return the first matching evaluated option" do
> + @param2.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" }.returns(true)
> +
> @value2.stubs(:safeevaluate).with(@scope).returns(:result)
> +
> + @selector.evaluate(@scope).should == :result
> + end
> +
> + it "should evaluate the default option if none matched"
> do
> +
> @param1.stubs(:is_a?).with(Puppet::Parser::AST::Default).returns(true)
> +
> @value1.expects(:safeevaluate).with(@scope).returns(@param1)
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should return the default evaluated option if none
> matched" do
> + result = stub 'result'
> +
> @param1.stubs(:is_a?).with(Puppet::Parser::AST::Default).returns(true)
> + @value1.stubs(:safeevaluate).returns(result)
> +
> + @selector.evaluate(@scope).should == result
> + end
> +
> + it "should return nil if nothing matched" do
> + @selector.evaluate(@scope).should be_nil
> + end
> +
> + it "should delegate matching to evaluate_match" do
> + @param1.expects(:evaluate_match).with { |*arg|
> arg[0] == "value" and arg[1] == @scope }
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should transmit the sensitive parameter to
> evaluate_match" do
> + Puppet.stubs(:
> []).with(:casesensitive).returns(:sensitive)
> + @param1.expects(:evaluate_match).with { |*arg|
> arg[2][:sensitive] == :sensitive }
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should transmit the AST file and line to
> evaluate_match" do
> + @selector.file = :file
> + @selector.line = :line
> + @param1.expects(:evaluate_match).with { |*arg|
> arg[2][:file] == :file and arg[2][:line] == :line }
> +
> + @selector.evaluate(@scope)
> + end
> +
> +
> + it "should evaluate the matching param" do
> + @param1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> +
> + @value1.expects(:safeevaluate).with(@scope)
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should return this evaluated option if it matches" do
> + @param1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> +
> @value1.stubs(:safeevaluate).with(@scope).returns(:result)
> +
> + @selector.evaluate(@scope).should == :result
> + end
> +
> + it "should unset scope ephemeral variables after option
> evaluation" do
> + @param1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> +
> @value1.stubs(:safeevaluate).with(@scope).returns(:result)
> +
> + @scope.expects(:unset_ephemeral_var)
> +
> + @selector.evaluate(@scope)
> + end
> +
> + it "should not leak ephemeral variables even if
> evaluation fails" do
> + @param1.stubs(:evaluate_match).with { |*arg| arg[0]
> == "value" and arg[1] == @scope }.returns(true)
> + @value1.stubs(:safeevaluate).with(@scope).raises
> +
> + @scope.expects(:unset_ephemeral_var)
> +
> + lambda { @selector.evaluate(@scope) }.should
> raise_error
> + end
> +
> + it "should fail if there is no default" do
> + @selector.expects(:fail)
> +
> + @selector.evaluate(@scope)
> + end
> + end
> +
> + end
> +end
> diff --git a/test/data/snippets/casestatement.pp b/test/data/
> snippets/casestatement.pp
> index 9559828..66ecd72 100644
> --- a/test/data/snippets/casestatement.pp
> +++ b/test/data/snippets/casestatement.pp
> @@ -56,3 +56,10 @@ case $yay {
> default: { file { "/tmp/existsfile5": mode => 711, ensure =>
> file } }
>
> }
> +
> +$regexvar = "exists regex"
> +case $regexvar {
> + "no match": { file { "/tmp/existsfile6": mode => 644, ensure =>
> file } }
> + /(.*) regex$/: { file { "/tmp/${1}file6": mode => 755, ensure
> => file } }
> + default: { file { "/tmp/existsfile6": mode => 711, ensure =>
> file } }
> +}
> diff --git a/test/data/snippets/selectorvalues.pp b/test/data/
> snippets/selectorvalues.pp
> index cd8cf77..d80d26c 100644
> --- a/test/data/snippets/selectorvalues.pp
> +++ b/test/data/snippets/selectorvalues.pp
> @@ -34,9 +34,16 @@ $mode6 = $mode5 ? {
> 755 => 755
> }
>
> +$mode7 = "test regex" ? {
> + /regex$/ => 755,
> + default => 644
> +}
> +
> +
> file { "/tmp/selectorvalues1": ensure => file, mode => $mode1 }
> file { "/tmp/selectorvalues2": ensure => file, mode => $mode2 }
> file { "/tmp/selectorvalues3": ensure => file, mode => $mode3 }
> file { "/tmp/selectorvalues4": ensure => file, mode => $mode4 }
> file { "/tmp/selectorvalues5": ensure => file, mode => $mode5 }
> file { "/tmp/selectorvalues6": ensure => file, mode => $mode6 }
> +file { "/tmp/selectorvalues7": ensure => file, mode => $mode7 }
> diff --git a/test/language/snippets.rb b/test/language/snippets.rb
> index cfca10e..5c7805c 100755
> --- a/test/language/snippets.rb
> +++ b/test/language/snippets.rb
> @@ -237,6 +237,7 @@ class TestSnippets < Test::Unit::TestCase
> /tmp/existsfile3
> /tmp/existsfile4
> /tmp/existsfile5
> + /tmp/existsfile6
> }
>
> paths.each { |path|
> @@ -281,7 +282,7 @@ class TestSnippets < Test::Unit::TestCase
> end
>
> def snippet_selectorvalues
> - nums = %w{1 2 3 4 5}
> + nums = %w{1 2 3 4 5 6 7}
> files = nums.collect { |n|
> "/tmp/selectorvalues%s" % n
> }
> diff --git a/test/lib/puppettest/parsertesting.rb b/test/lib/
> puppettest/parsertesting.rb
> index 8186bf3..dee38eb 100644
> --- a/test/lib/puppettest/parsertesting.rb
> +++ b/test/lib/puppettest/parsertesting.rb
> @@ -33,6 +33,11 @@ module PuppetTest::ParserTesting
> def safeevaluate(*args)
> evaluate()
> end
> +
> + def evaluate_match(othervalue, scope, options={})
> + value = evaluate()
> + othervalue == value
> + end
> end
>
> def astarray(*args)
> --
> 1.6.0.2
>
>
> >
--
This space intentionally has nothing but text explaining why this
space has nothing but text explaining that this space would otherwise
have been left blank, and would otherwise have been left blank.
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
