+1, with comments below. On Dec 7, 2009, at 10:17 PM, Markus Roberts wrote:
> This is the sketch of what should be a pretty straight forward fix > for #2863: > > * Replace the direct @var access with an initializing getter > * Rename it from @tags_hash to @tags_list since it's not a hash > * Do the same with the otherwise identical params setup. > * Eliminate the now-redundant external initialization for params. > > The only hitch is a single call to "get_params_hash" on line 197 of > lib/puppet/rails/resource.rb -- I can not find where this is coming > from (grep throws up it's hands, there's no field I can see in the > schema that would drive a rails magic method to produce it, and so > on). I believe this is used in testing, if at all. The 'parameters' method (AFAIR) isn't used anywhere in normal code paths, but I found it useful either in unit tests or real-world testing to figure out what was actually going on without all of the complication of ActiveRecord objects. > If it's simply an additional bug, fine. But I'm being low confidence > today (trouble breathing) and want to get other eyeballs on it. So > my questions: > > * Is it just another unrelated a bug, and if so what would need to > be done to exhibit it? > * Is it dead / unreachable code? > * Is there some magic I'm missing and if so does that render my > renaming problematic (i.e., they need to be called xyz_hash to > mate with some other process)? I'm pretty positive that method is just there to produce a simple hash of the parameters, and even then, I can't find it being used in either lib or spec, so I'm comfortable just deleting that method. > Signed-off-by: Markus Roberts <[email protected]> > --- > lib/puppet/rails/host.rb | 9 ++------- > lib/puppet/rails/resource.rb | 30 ++++++++++++++++++------------ > 2 files changed, 20 insertions(+), 19 deletions(-) > > diff --git a/lib/puppet/rails/host.rb b/lib/puppet/rails/host.rb > index d66fd2e..6b057dd 100644 > --- a/lib/puppet/rails/host.rb > +++ b/lib/puppet/rails/host.rb > @@ -172,11 +172,6 @@ class Puppet::Rails::Host < ActiveRecord::Base > end > > def find_resources_parameters_tags(resources) > - # initialize all resource parameters > - resources.each do |key,resource| > - resource.params_hash = [] > - end > - > find_resources_parameters(resources) > find_resources_tags(resources) > end > @@ -294,7 +289,7 @@ class Puppet::Rails::Host < ActiveRecord::Base > > # assign each loaded parameters/tags to the resource it > belongs to > params.each do |param| > - > resources[param['resource_id']].add_param_to_hash(param) if > resources.include?(param['resource_id']) > + > resources[param['resource_id']].add_param_to_list(param) if > resources.include?(param['resource_id']) > end > end > > @@ -302,7 +297,7 @@ class Puppet::Rails::Host < ActiveRecord::Base > tags = > Puppet::Rails::ResourceTag.find_all_tags_from_host(self) > > tags.each do |tag| > - resources[tag['resource_id']].add_tag_to_hash(tag) if > resources.include?(tag['resource_id']) > + resources[tag['resource_id']].add_tag_to_list(tag) if > resources.include?(tag['resource_id']) > end > end > > diff --git a/lib/puppet/rails/resource.rb b/lib/puppet/rails/ > resource.rb > index 984bdc0..b76f9e7 100644 > --- a/lib/puppet/rails/resource.rb > +++ b/lib/puppet/rails/resource.rb > @@ -63,22 +63,28 @@ class Puppet::Rails::Resource < ActiveRecord::Base > unserialize_value(self[:title]) > end > > - def add_param_to_hash(param) > - @params_hash ||= [] > - @params_hash << param > + def params_list > + @params_list ||= [] > end > > - def add_tag_to_hash(tag) > - @tags_hash ||= [] > - @tags_hash << tag > + def params_list=(params) > + @params_list = params > end > > - def params_hash=(hash) > - @params_hash = hash > + def add_param_to_list(param) > + @params_list << param > end > > - def tags_hash=(hash) > - @tags_hash = hash > + def tags_list > + @tags_list ||= [] > + end > + > + def tags_list=(tags) > + @tags_list = tags > + end > + > + def add_tag_to_list(tag) > + tags_list << tag > end > > def [](param) > @@ -116,7 +122,7 @@ class Puppet::Rails::Resource < ActiveRecord::Base > db_params = {} > > deletions = [] > - @params_hash.each do |value| > + params_list.each do |value| > # First remove any parameters our catalog resource > doesn't have at all. > deletions << value['id'] and next unless > catalog_params.include?(value['name']) > > @@ -156,7 +162,7 @@ class Puppet::Rails::Resource < ActiveRecord::Base > in_db = [] > deletions = [] > resource_tags = resource.tags > - @tags_hash.each do |tag| > + tags_list.each do |tag| > deletions << tag['id'] and next unless > resource_tags.include?(tag['name']) > in_db << tag['name'] > end > -- > 1.6.4 > > -- > > 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 > . > > -- Levy's Law: The truth is always more interesting than your preconception of what it might be. --------------------------------------------------------------------- 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.
