Haml engine local assigns modifies Object.

In Haml::Engine#render:
<pre>
176     def render(scope = Object.new, &block)
177       scope_object = scope
178       buffer = Haml::Buffer.new(options)
179
180       local_assigns = options[:locals]
181
182       # Get inside the view object's world
183       scope_object.instance_eval do
184         # Set all the local assigns
185         local_assigns.each do |key,val|
186           self.class.send(:define_method, key) 
187         end
188       end
</pre>

Notice that scope and scope_object are, by default, Objects.
When
local_assigns are...assigned...the Object class has new
methods
defined on it. This is *bad*.

I suggest that we replace it with this:
<pre>
m = Module.new
locals_assigns.each { |k, v| m.send(:define_method, k) 
}
scope_object.send(:extend, m)
</pre>

Thoughts?

Nick


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the
Google Groups "Haml" group.
To post to this group, send email to hamlgooglegroups.com
To unsubscribe from this group, send email to
haml-unsubscribegooglegroups.com
For more options, visit this group at http://grou
ps.google.com/group/haml?hl=en
-~----------~----~----~----~------~----~------~--~---

Or, if you want an uber-one-liner:

scope_object.send(:extend, options[:locals].inject(Module.new) { |m,
k, v| m.send(:define_method, k)  })

Is it just me, or could Haml use some code-cleanup?

Nick

On Apr 12, 6:34 pm, "Nathan Weizenbaum"
<nex...gmail.com> wrote:
> You're right, it should be like that. I don't know how
it snuck through.
> I'll fix it later today.
>
> - Nathan
>
> On 4/12/07, Nick Howell <nlhow...gmail.com> wrote:
>
>
>
> > In Haml::Engine#render:
> > <pre>
> > 176     def render(scope = Object.new,
&block)
> > 177       scope_object = scope
> > 178       buffer = Haml::Buffer.new(options)
> > 179
> > 180       local_assigns = options[:locals]
> > 181
> > 182       # Get inside the view object's world
> > 183       scope_object.instance_eval do
> > 184         # Set all the local assigns
> > 185         local_assigns.each do |key,val|
> > 186           self.class.send(:define_method, key)

> > 187         end
> > 188       end
> > </pre>
>
> > Notice that scope and scope_object are, by
default, Objects. When
> > local_assigns are...assigned...the Object class
has new methods
> > defined on it. This is *bad*.
>
> > I suggest that we replace it with this:
> > <pre>
> > m = Module.new
> > locals_assigns.each { |k, v|
m.send(:define_method, k)  }
> > scope_object.send(:extend, m)
> > </pre>
>
> > Thoughts?
>
> > Nick


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the
Google Groups "Haml" group.
To post to this group, send email to hamlgooglegroups.com
To unsubscribe from this group, send email to
haml-unsubscribegooglegroups.com
For more options, visit this group at http://grou
ps.google.com/group/haml?hl=en
-~----------~----~----~----~------~----~------~--~---

That actually breaks... k ends up with a two element
key/value array and 
v ends up with nil. Also, inject takes the return value of
the last 
block as the memo value for the next block, so you'd have to
do

scope_object.extend(options[:locals].inject(Module.new) { |m, k|
m.send(:define_method, k[0]) { k[1] }; m })

As to the messiness of the codebase, I think to some extent
it's 
inevitable. As we add more features, write more stuff to
handle edge 
cases, etc. it's not always possible to keep it elegant. Not
to mention 
that, as humans, we tend to make stupid mistakes from time
to time (like 
this one... heh).

As part of the 2.0 release, I'm planning to do some serious
reworking of 
the Engine/Buffer rendering system. This is primarily to
increase 
efficiency by making as much rendering as possible happen
before the 
template is cached, but it'll also offer a good opportunity
to take a 
good look at the code that's currently doing the rendering
and clean it 
up as much as possible.

That's not to say that it wouldn't be worthwhile to do some
cleaning 
now, though. If you're interested, by all means go for it.

- Nathan

Nick Howell wrote:
> Or, if you want an uber-one-liner:
>
> scope_object.send(:extend, options[:locals].inject(Module.new) { |m,
> k, v| m.send(:define_method, k)  })
>
> Is it just me, or could Haml use some code-cleanup?
>
> Nick
>
> On Apr 12, 6:34 pm, "Nathan Weizenbaum"
<nex...gmail.com> wrote:
>   
>> You're right, it should be like that. I don't know
how it snuck through.
>> I'll fix it later today.
>>
>> - Nathan
>>
>> On 4/12/07, Nick Howell <nlhow...gmail.com> wrote:
>>
>>
>>
>>     
>>> In Haml::Engine#render:
>>> <pre>
>>> 176     def render(scope = Object.new,
&block)
>>> 177       scope_object = scope
>>> 178       buffer = Haml::Buffer.new(options)
>>> 179
>>> 180       local_assigns = options[:locals]
>>> 181
>>> 182       # Get inside the view object's world
>>> 183       scope_object.instance_eval do
>>> 184         # Set all the local assigns
>>> 185         local_assigns.each do |key,val|
>>> 186           self.class.send(:define_method,
key) 
>>> 187         end
>>> 188       end
>>> </pre>
>>>       
>>> Notice that scope and scope_object are, by
default, Objects. When
>>> local_assigns are...assigned...the Object class
has new methods
>>> defined on it. This is *bad*.
>>>       
>>> I suggest that we replace it with this:
>>> <pre>
>>> m = Module.new
>>> locals_assigns.each { |k, v|
m.send(:define_method, k)  }
>>> scope_object.send(:extend, m)
>>> </pre>
>>>       
>>> Thoughts?
>>>       
>>> Nick
>>>       
>
>
> >
>
>   


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the
Google Groups "Haml" group.
To post to this group, send email to hamlgooglegroups.com
To unsubscribe from this group, send email to
haml-unsubscribegooglegroups.com
For more options, visit this group at http://grou
ps.google.com/group/haml?hl=en
-~----------~----~----~----~------~----~------~--~---

Doh. 'spose I shoulda tested it. Especially since that was
the first
time I'd ever used inject. I feel like an ass now.

Thanks for getting that in so quick; I post and the next
time I update
the fix is in.

I'll keep my eyes out for anything that looks like an easy
fix, and
post up if I find anything interesting.

I've actually been kinda interested in modifying the Engine
through
redefining certain methods; it would be nice if the compile
and render
methods could be factored into sub-methods (like an
assign_locals
method, in this case), to allow for easier overrides. If
you're gonna
be reworking it pretty significantly for 2.0, I won't bother
messing
with it now.

I'll definitely be looking forward to performance
improvements, though
I've been pleasantly surprised by the current implementation
(I'd read
in a few places that Haml was a lot slower than ERB).

Nick

On Apr 15, 9:26 pm, Nathan Weizenbaum <nex...gmail.com> wrote:
> That actually breaks... k ends up with a two element
key/value array and
> v ends up with nil. Also, inject takes the return value
of the last
> block as the memo value for the next block, so you'd
have to do
>
> scope_object.extend(options[:locals].inject(Module.new) { |m, k|
m.send(:define_method, k[0]) { k[1] }; m })
>
> As to the messiness of the codebase, I think to some
extent it's
> inevitable. As we add more features, write more stuff
to handle edge
> cases, etc. it's not always possible to keep it
elegant. Not to mention
> that, as humans, we tend to make stupid mistakes from
time to time (like
> this one... heh).
>
> As part of the 2.0 release, I'm planning to do some
serious reworking of
> the Engine/Buffer rendering system. This is primarily
to increase
> efficiency by making as much rendering as possible
happen before the
> template is cached, but it'll also offer a good
opportunity to take a
> good look at the code that's currently doing the
rendering and clean it
> up as much as possible.
>
> That's not to say that it wouldn't be worthwhile to do
some cleaning
> now, though. If you're interested, by all means go for
it.
>
> - Nathan
>
> Nick Howell wrote:
> > Or, if you want an uber-one-liner:
>
> > scope_object.send(:extend, options[:locals].inject(Module.new) { |m,
> > k, v| m.send(:define_method, k)  })
>
> > Is it just me, or could Haml use some
code-cleanup?
>
> > Nick
>
> > On Apr 12, 6:34 pm, "Nathan Weizenbaum"
<nex...gmail.com> wrote:
>
> >> You're right, it should be like that. I don't
know how it snuck through.
> >> I'll fix it later today.
>
> >> - Nathan
>
> >> On 4/12/07, Nick Howell <nlhow...gmail.com> wrote:
>
> >>> In Haml::Engine#render:
> >>> <pre>
> >>> 176     def render(scope = Object.new,
&block)
> >>> 177       scope_object = scope
> >>> 178       buffer =
Haml::Buffer.new(options)
> >>> 179
> >>> 180       local_assigns = options[:locals]
> >>> 181
> >>> 182       # Get inside the view object's
world
> >>> 183       scope_object.instance_eval
do
> >>> 184         # Set all the local assigns
> >>> 185         local_assigns.each do
|key,val|
> >>> 186          
self.class.send(:define_method, key) 
> >>> 187         end
> >>> 188       end
> >>> </pre>
>
> >>> Notice that scope and scope_object are, by default, Objects. When
> >>> local_assigns are...assigned...the Object
class has new methods
> >>> defined on it. This is *bad*.
>
> >>> I suggest that we replace it with this:
> >>> <pre>
> >>> m = Module.new
> >>> locals_assigns.each { |k, v|
m.send(:define_method, k)  }
> >>> scope_object.send(:extend, m)
> >>> </pre>
>
> >>> Thoughts?
>
> >>> Nick


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the
Google Groups "Haml" group.
To post to this group, send email to hamlgooglegroups.com
To unsubscribe from this group, send email to
haml-unsubscribegooglegroups.com
For more options, visit this group at http://grou
ps.google.com/group/haml?hl=en
-~----------~----~----~----~------~----~------~--~---