Dustin, thanks for the review!
(more comments inline)
Regards
- henrik
On 2013-23-08 3:11, Dustin Mitchell wrote:
Picking up discussion of this ARM, now that I've found time to look
deeply at it.
This may be silly, but "Extended Puppet Templates" seems more logically
abbreviated "EPT" - why "EPP"?
Does it say "Extended Puppet Templates" somewhere? The idea is to do the
same as in Ruby, i.e. "Embedded Ruby" = ERP, "Embedded Puppet" = EPP.
Could you include a link to the Puppet DSL in the text? It's easy to
think that that means Puppet itself (e.g., resource declarations), when
really it is a form of Ruby (I think). The link will add context.
Sorry, but I don't get your question. EPP is to Puppet what ERB is to
ruby. The EPP content is in a file, typically with extension .epp, but
can be anything except .pp (which is naturally "not embedded puppet code").
But, I think you are asking something else...
The paragraph beginning with "Also note that it is not possible" is
confusing since template parameters have not yet been introduced at that
point in the text - perhaps move it down to the next section?
"If someone chooses to use these questionable expressions inside a
template, there is no real harm; only poor design." -- I think we will
want to validate for this at some point, and it will be much easier to
do so if the documentation said "DO NOT DO THIS" from the beginning,
rather than "Go ahead, but it's poor design".
Yes, agree.
As for @ vs. $ -- how difficult would it be to write erb2epp.rb to do
the search-and-replace in a syntax-sensitive fashion? If that's just an
hour's hacking, then it might be a very useful addition, especially when
ERB is deprecated.
True, but it can be "anything" in the ruby logic.
Using a tool like Geppetto it is trivial to search replace and also
review what is being changed.
BTW, I think the ARM will receive a round of edits as preparation for
writing the real documentation. So thanks for identifying where
improvements can be made.
As for validation of filetypes, I think this is a good idea, but I don't
like the <%- ($x, $y) :html -%> syntax for this. I'd prefer something a
little more explicit and expandable, perhaps
<%- ($x, $y) syntax => html -%>
The idea was to mimic the same specification in ARM-4 Heredoc, where the
syntax is ':syntax'.
I am not super happy with the somewhat magic <%- () -%> construct. It
allowed me to reuse parameter parsing. Did not spend too much time on
figuring out alternatives, but the ones that came to mind I thought were
worse.
As for scope, I think that the fact that ERBs inherit their invoker's
scope is a source of bugs and unintentional use of scoped variables.
However, restricted scope would make it more difficult to convert ERBs.
Ideally, this would be an option to the EPP functions, defaulting to
restricted scope, so that conversion from ERB to EPP involves explicitly
adding that option and only removing it when all variables are passed.
Your comment indicates that's difficult, though, so I'd leave it to you
to measure the effort vs. future-proofing tradeoff.
Very good point. It is currently a bit of a mess since the original
template functions performs join of results if more than one file is
given. Lots of parameter overloading to make the function to many
different things is not good either. But, I totally agree that it should
be the caller's responsibility to give access to the current scope or not.
Whether it should be restricted scope of not by default I don't know. It
could be a chore to have to write a hash with name to value associations.
There is a Redmine issue where it is discussed if it is good or bad that
the template functions can join results from multiple templates. I would
prefer if it was just one, and instead call a join function if that
behavior is needed.
Finally, it'd be nice to see a fully worked example with some iteration
over lists, conditionals, and maybe some small Ruby tricks like
shuffling lists or mapping. The only lengthy code example is illegal.
Yes, more real world examples is always good. Maybe take some existing
in ERB and convert them. Will certainly be made for the finished
documentation. (The ARM is after all input to implementation and
documentation).
Most of these suggestions concern the text of the ARM itself. I haven't
looked at the implementation, but assuming it works as advertised,
Yes, it works as advertized; the syntax for syntax checking is not
implemented, and (naturally, it does not yet call out to perform any
such checks).
I'm
100% in favor of this.
Super!
- henrik
--
You received this message because you are subscribed to the Google Groups "Puppet
Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/puppet-dev.
For more options, visit https://groups.google.com/groups/opt_out.
