Hi Rein,
On Wed, 2009-10-14 at 15:33 -0700, Rein Henrichs wrote:
> In the following code:
>
> > + def add_require(required)
> > + add_to(@requires, required)
> > + end
>
> This redefines the add_require method, which has RDoc::Context, where
> it has different behavior. Perhaps a comment indicating that this is
> due to the behavior of require in puppet code would help unsuspecting
> readers?
While I do fundamentally agree, I think those who'll read this code will
understand quite quickly (as you've done) that I (ab)used RDoc in a way
that we're not supposed to.
For instance, look how a Puppet definition is mapped to a Ruby method...
> In the following code:
>
> + # This module is used to generate a referenced full name list of
> ContextUser
> + module ReferencedListBuilder
> + def build_referenced_list(list)
> + res = []
> + list.each do |i|
> + ref = AllReferences[i.name]
> + ref = @context.find_symbol(i.name)
> + ref = ref.viewer if ref
> + name = i.respond_to?(:full_name) ? i.full_name : i.name
> + h_name = CGI.escapeHTML(name)
> + if ref and ref.document_self
> + path = url(ref.path)
> + res << { "name" => h_name, "aref" => path }
> + else
> + res << { "name" => h_name }
> + end
> + end
> + res
> + end
> + end
>
> The second ref = line inside the each block appears to reassign to ref
> whether it was found in AllReferences or not. This strikes me as
> probably unintended. Is the following closer to your intent?
>
> ref = AllReferences[i.name] || @context.find_symbol(i.name)
Probably. It is a copy/pasted section from RDoc, which I edited to
remove unneeded things, so I might have missed a couple of things.
I'll rework this.
> In the following code:
>
> + case stmt.name
> + when "include"
> + stmt.arguments.each do |included|
> + Puppet.debug "found include: %s" % included.value
> +
> container.add_include(Include.new(included.value, stmt.doc))
> + end
> + when "require"
> + stmt.arguments.each do |required|
> + Puppet.debug "found require: %s" % required.value
> +
> container.add_require(Include.new(required.value, stmt.doc))
> + end
> + end
>
> I don't have a problem with this, but how do you feel about something
> like the following?
>
> stmt.arguments.each do |argument|
> Puppet.debug "found %s: %s" % [argument.name, argument.value]
> container.send("add_#{argument.name}", Include.new(argument.value,
> stmt.doc))
> end
The code above is wrong (ie using argument.name where it should use
stmt.name).
And, even corrected, it isn't equivalent, stmt.name can be any Puppet
function name, but we know only include and require. This will throw an
unknown method exception if the parser encounters any other function.
if ['include', 'require'].include?(stmt.name)
stmt.arguments.each do |argument|
Puppet.debug "found %s: %s" % [stmt.name, argument.value]
container.send("add_#{stmt.name}", Include.new(argument.value,
stmt.doc))
end
end
is more correct.
> > [Patch snipped]
--
Brice Figureau
Follow the latest Puppet Community evolutions on www.planetpuppet.org!
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
