Hi,
I'm crafting a new version of this patch series, and as such I'm
depiling the comments.
On 27/07/09 6:35, Luke Kanies wrote:
> 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?
no, I think it is for:
case $foo {
a,b : { ... }
}
a and b are aggregated in an ASTArray in the grammar.
And the client code doesn't know how to deal with an ASTArray
(especially evaluate_match). So we have to process the case options one
by one.
We _could_ allow array matching in evaluate_match, if you insist, but
it's more work for little benefits as we master the evaluate_match
client site.
> 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?
The syntax
case $foo {
a, [b, c]: { ... }
}
doesn't seem valid, as I read the grammar (but I didn't check).
>> # 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.
I rewrote the whole thing to return asap when we have a match.
I'll repost the patch later tonight.
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
