On Fri, Dec 4, 2009 at 2:47 AM, Brice Figureau
<[email protected]> wrote:
> Hi,
>
> I tried to read the whole patch (please remove parser.rb patch next time
> it takes 90% of the patch for no added value) and it looks (really)
> good, so I'm +1 if that was needed.
Yeah, I knew-but-didn't-think-about parser.rb until I'd done "rake
mail_patches". What's the recommended way of pulling that out (is
there an "exclude from diff" setting somewhere, or...?)
> It looks like you allow "expression"s in the ${ } tag, which is really
> nice.
Yes. I'd like to allow slightly more flexibility in this regard, say
allowing expressions in arrays (it would add expressiveness and
simplify the grammar with little to no risk of abuse), but that will
have to wait for a second pass..
>> + class Concat < AST::Leaf
>> + def evaluate(scope)
>> + �[email protected] { |x| x.evaluate(scope) }.join
>> + end
>> +
>> + def to_s
>> + "concat(#[email protected](',')})"
>> + end
>
> Those to_s are mainly used by Puppetdoc RDoc to recreate the puppet
> manifest from the AST node. Please change this method to return
> something more similar to the originally parsed string otherwise all
> strings will look strange in puppetdoc.
>
> But since you said in your comment it will disappear soon in favor of
> Futures, you can leave it now.
Hmmm. I should probably fix it anyway, if only so it doesn't fall
into the "I don't remember why this is here but we'd better maintain
that functionality" trap.
> BTW, I'm wondering how puppetdoc will deal with future
> "interpretation" :-(
> I'll look at that when you'll release the future patch.
I'm thinking it should be OK with it. Under static analysis they're
just named variables.
>>
>> + # MQR: Why not just alias?
>
> This is certainly my fault. I wasn't aware of method aliasing when I
> wrote this, and this looked like a pattern already used in puppet...
> You have my blessing to rewrite it the way you want.
>
>> %w{skip accumulate}.each do |method|
>> define_method(method+"?") do
>> self.send(method)
I figured it was probably something like that. I planning on doing a
fair amount of that sort of code reduction at some point, if only on
the principle that the fewer corners in the code, the fewer places
there are for bugs to hide.
>> +__ = nil
> What's the use of this?
>> it "should not consider escaped slashes to be the end of a regex" do
>> - tokens_scanned_from("$x =~ /this \\/ foo/").last[:value].should
>> == Regexp.new("this / foo")
>> + tokens_scanned_from("$x =~ /this \\/ foo/").should
>> be_like(__,__,[:REGEX,%r{this / foo}])
> Ah ok, got it. You can dismiss my previous question...
Yeah, that's an idiom I was playing around with, not sure if I like it
yet. That whole test suite is slightly bothersome; good on coverage,
but too focused (IMHO) on the _how_ part--in many cases it tests the
implementation details rather than the results. I started to do a
broader refactoring but stopped myself at doing what I needed for
presently planned changes.
I also keep tripping over the minor differences between Regexp and Regex.
-- Markus
--
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.
