>> > 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.
A kindred spirit! I should start doing that as a matter of principle.
>> > 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.
*smile* I know how that goes. The "someday" that never comes.
>> >> 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.
I tried a few things in that direction, but include doesn't quite do
it because the results actually have structured data (with file and
line information) and we really are only caring about the value here.
In the same exploration I also fell afoul of this sweet little problem
in rspec:
tokens_scanned_from("bob").should be_any { |name,data| name ==
:REGEX and data[:value] == %r{this / foo} }
will pass. In fact:
[1,2,3].should be_all { |x| x < 0 }
will always pass. Likewise with be_any. They always pass, provided
merely that the result responds to any? / all?.
>> 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...
Yeah. Extensive test changes should probably be a patch unto themselves.
>> 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.
*grin* An infelicitous name that can't be traced to Luke? Say it isn't so!
-- 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.
