On Fri, Dec 17, 2010 at 12:00 PM, Luke Kanies <[email protected]> wrote:
> Looks good, with one comment:
>
> I recommend that the apply_parameter method act on the created event, such
> as by adding it to the 'events' array, or, preferably, having an 'add_event'
> method that does this.
>
I assume you are responding to the unfortunate code duplication between
these lines:
events << apply_parameter(ensure_param, current_values[:ensure],
audited_params.include?(:ensure), historical_values[:ensure])
synced_params << :ensure
and these:
events << apply_parameter(param, current_values[param.name],
audited_params.include?(param.name), historical_values[param.name])
synced_params << param.name
I considered a refactor along the lines you suggest, but I decided against
it for the following reason: if apply_parameter were responsible for adding
the event to the events array, then perform_changes would be relying for its
correctness on the knowledge that apply_parameter always adds exactly one
event to the event array, corresponding to the parameter in question.
Although there is a little bit of code duplication in the patch as
submitted, it seems like a small price to pay for the fact that the output
of apply_parameter is explicit, so you only need to look at the
perform_changes method to understand what events will be produced.
--
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.
