On Sep 16, 2009, at 11:05 AM, Brice Figureau wrote: > > On 16/09/09 19:36, Markus Roberts wrote: >> >> >> On Wed, Sep 16, 2009 at 8:54 AM, Brice Figureau >> <[email protected] <mailto:brice- >> [email protected]>> >> wrote: >> >> >> On Wed, 2009-09-16 at 07:47 -0700, Markus wrote: >>> On Wed, 2009-09-16 at 09:49 +0200, Brice Figureau wrote: >>>> On Tue, 2009-09-15 at 15:16 -0700, Luke Kanies wrote: >>>>> I can't think of a better solution, but I'm not real stoked about >>>>> 'node_exists?' returning something other than a boolean. >>>> >>>> Yes, I had the same question. But couldn't find a better name which >>>> didn't involve renaming all other call sites. I tried >> node_by_name or >>>> node_by_equality, but none where really satisfying either. >>> >>> Rename any call sites that use the return value of 'node_exists?' as >>> anything other than a boolean. If this is all of them, then the >> method >>> should be renamed. If this is only some of them, then there are >> really >>> two methods, one of which (probably 'node_exists?') is a thin >> wrapper on >>> the other. >> >> Yes, the method should be renamed. I suck at naming things. When >> I sent >> the patch (crafted in a couple of minutes), I knew I'd have some >> objection. >> >> My issue is that there are lots of clients of "node" (which in >> fact does >> a the job of "match_node"), and since there is only one client of >> "node_exists?" this was the simplest solution. >> And I don't have much time to devote to Puppet coding before the >> 0.25.1 >> release, so I'd prefer to find a good name for node_exists? >> instead of >> doing the reverse (ie s/node/match_node/g). >> >> Now, since I suck at naming things, can someone give me >> alternative >> names for this method? >> >>> There's no reason a method like 'foo?' can't return any >> reasonable value >>> it likes, but you should only count on the boolean traits of the >> result >>> (if you want to force it to be a boolean you can always use the >> '!!foo?' >>> idiom). >>> >>> I'm planning some code-quality sweeps, and this is one of the target >>> issues. >> >> It will be interesting to see the results. >> >> >> >> I'd probably write something like: >> >> def node_called(name) >> @nodes[check_name(name)] >> end >> >> def node_matching(name) >> node_called(name) || @node_list.select { |n| n.regexp? and >> n.match(name)}.find { |n| @nodes[n] } >> end >> >> Where node_matching() replaced the old node() and node_called() >> replaced >> node_exists(). > > Also, loaded_code being new in Puppet nobody seems to use it directly. > All access I could find are proxied through the Parser. That means the > above change is safe for 0.25.1, as long as we don't change the > Parser API.
Yeah - LoadedCode is currently an entirely internal API. As we successfully extract the hostclass/definition/node out of the AST layer, we'll have exposed APIs for creating and interacting with instances of them, which would mean either directly exposing the LoadedCode API or (more likely) providing a thin wrapper on top of it. -- Charm is a way of getting the answer yes without asking a clear question. -- Albert Camus --------------------------------------------------------------------- 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 -~----------~----~----~----~------~----~------~--~---
