On Oct 12, 2010, at 3:21 PM, Paul Berry wrote: > On Tue, Oct 12, 2010 at 12:46 PM, Jesse A Wolfe <[email protected]> wrote: > # Make an instance of our resource type. This is only possible > # for those classes and nodes that don't have any arguments, and is > # only useful for things like the 'include' function. > > The bit about "don't have any arguments" seems stale, doesn't it? > > You're right. I will fix this.
My knowledge might quite possibly be out of date here, but this came down to having painfully separate code paths for resource creation - using 'include' to create a resource with no parameters, and using an AST resource to create a resource instance with (possibly zero) parameters. Basically, this method needed to exist when we were using something other than an AST resource for resource creation, although I can't remember much of the context beyond that. Obviously, the facts on the ground may have changed. > - def mk_plain_resource(scope) > + def mk_singleton_resource(scope) > > I agree that "mk_plain_resource" is not a good name, but I'm not sure if > "singleton resource" is much clearer - is that a new coinage or are we > already using it as jargon elsewhere? (I recognize that we've got some naming > work to do outside the scope of this patch, we need a more descriptive word > for ["classes" "nodes" and "defined types"] than "Resource::Type") > > and if you're going to be renaming it anyway, let's lose the "mk". I think it > stands for "make", but the abbreviation is unnecessary, and "make" is a > nearly meaningless word in this context. > > > The two pieces of information I want to be captured (or at least hinted at) > in the name are the fact at (a) the method both creates the resource and > installs it in the catalog--it's not just an accessor, and (b) the method is > idempotent--it doesn't do anything if the resource has already been created > and installed in the catalog. (a) is what motivated keeping "mk" in the > name, and (b) is what motivated the word "singleton". > > How would you feel about calling it "ensure_in_catalog"? > > + if ['Class', 'Node'].include? resource.type > + scope.catalog.tag(*resource.tags) > + end > > I'd weakly-prefer this to be extracted into a method, too. > > Ok, I don't see any harm in that. I'll fold that change in. -- The trouble with the world is that the stupid are cocksure and the intelligent are full of doubt. -- Bertrand Russell --------------------------------------------------------------------- Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199 -- 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.
