After thinking it through, I agree that #evaluate should be idempotent. I don't want to lose the tests for #builtin_type? and #virtual? though. I'm not convinced that it makes sense to fold them into #evaluate - currently built-in types raise an error, and it would be a behavior change to make those a no-op. I'm not even sure what it means to evaluate virtual resources.
So... are you proposing we keep the old #unevaluated_resources method? After getting burned by it, I feel like I want to have it disappear. ~Jesse On Thu, Sep 30, 2010 at 11:17 AM, Markus Roberts <[email protected]>wrote: > Wouldn't it be a lot cleaner to make Resource#evaluate idempotent? I would > tend do do something like: > > diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb > index e34f284..6c217f7 100644 > --- a/lib/puppet/parser/resource.rb > +++ b/lib/puppet/parser/resource.rb > @@ -64,14 +64,16 @@ class Puppet::Parser::Resource < Puppet::Resource > > # Retrieve the associated definition and evaluate it. > def evaluate > - @evaluated = true > - if klass = resource_type and ! builtin_type? > - finish > - return klass.evaluate_code(self) > + if evaluated? > + #nothing to do > elsif builtin? > devfail "Cannot evaluate a builtin type (#{type})" > + elsif klass = resource_type > + @evaluated = true > + finish > + klass.evaluate_code(self) > else > - self.fail "Cannot find definition #{type}" > + fail "Cannot find definition #{type}" > end > end > > But it could be something as simple as: > > diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb > index e34f284..c007d4d 100644 > --- a/lib/puppet/parser/resource.rb > +++ b/lib/puppet/parser/resource.rb > @@ -64,6 +64,7 @@ class Puppet::Parser::Resource < Puppet::Resource > > # Retrieve the associated definition and evaluate it. > def evaluate > + return if evaluated? > @evaluated = true > if klass = resource_type and ! builtin_type? > finish > > My concern is that by handling it out of band we're fighting one set of > non-obvious side effects (evaluating resources can cause others to be > evaluated) by introducing another (fortran like business with flag variables > and nexting in a loop, etc.) and that makes the code more brittle than it > needs to be. > > Did you consider this alternative? If so, why was it rejected or if not, > would you consider it? > > -- M > ----------------------------------------------------------- > The power of accurate observation is > commonly called cynicism by those > who have not got it. ~George Bernard Shaw > ------------------------------------------------------------ > > -- > 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]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > -- 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.
