On Fri, 2009-12-04 at 08:31 -0800, Markus Roberts wrote:
> 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...?)
I don't use mail_patches. I git format-patch by hand, then review them
in an editor in which I also strip parser.rb. Then I manually git
send-email.
This way I control what I do.
> > 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..
When I added the expression, I refrained from putting them everywhere. I
wanted first to make sure it is useful and works as intended.
My plan was to move more syntax to expression (ie arrays, function
arguments, etc...), but I never did it.
> >> + class Concat < AST::Leaf
> >> + def evaluate(scope)
> >> + @value.collect { |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.
OK, that was I thought, but since I didn't really read the POC about
futures...
> >>
> >> + # 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.
I have mixed feelings about this, but it has the merit to show that
we'll get some unknown tokens.
Maybe something like this (pseudo code):
tokens_scanned_from("$x =~ /this \\/ foo/").should be_include([:REGEX,%
r{this / foo}]
But this doesn't capture the position, so that's not better.
> 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.
Too much test changes make patch difficult to read, so it's easy to miss
something...
> I also keep tripping over the minor differences between Regexp and Regex.
My bad again. If you want to rename those to ASTRegexp (or anything
else), go ahead.
--
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.
