Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

erb.rb: Add locals option to set local variables to `ERB#result` method #1618

Closed
wants to merge 1 commit into from

Conversation

4 participants
@k0kubun
Copy link
Member

commented May 18, 2017

lib/erb.rb Outdated
#
def result(b=new_toplevel)
def result(b=new_toplevel, locals: {})
unless locals.empty?

This comment has been minimized.

Copy link
@sorah

sorah May 18, 2017

Member

Do we need this branching

This comment has been minimized.

Copy link
@k0kubun

k0kubun May 18, 2017

Author Member

Became much cleaner, thanks! 5477a7d

This comment has been minimized.

Copy link
@nobu

nobu May 18, 2017

Member

I think the default should be nil and if locals.

This comment has been minimized.

Copy link
@k0kubun

k0kubun May 18, 2017

Author Member

I think the default should be nil and if locals.

I agreed because it may be faster. Fixed so c39a419.

@k0kubun k0kubun force-pushed the k0kubun:result-with-hash branch 2 times, most recently from 5477a7d to c39a419 May 18, 2017

@matthewd

This comment has been minimized.

Copy link

commented May 18, 2017

This will modify the locals inside the passed binding. That seems to be a surprising side-effect; if the caller intended to do that, they could've just used local_variable_set for themselves... accepting the hash here suggests that the extra variables will only be seen inside the template.

x = 1
ERB.new("<%= x %>").result(binding, locals: { x: 5 }) # => "5" ✅
x # => 5 😯
@k0kubun

This comment has been minimized.

Copy link
Member Author

commented May 18, 2017

This will modify the locals inside the passed binding. That seems to be a surprising side-effect; if the caller intended to do that, they could've just used local_variable_set for themselves...

Good catch. I'm thinking about how to fix this... I want to do the same thing as ce213d2 with proper API. (Of course this commit doesn't work for some cases...)

I managed to find a way to implement it ed1c1e5.

@k0kubun k0kubun force-pushed the k0kubun:result-with-hash branch 5 times, most recently from f1fc68b to caadf1b May 19, 2017

erb.rb: Add locals option to set local variables
to `ERB#result` method.

[ruby-core:55985] [Feature #8631]

@k0kubun k0kubun force-pushed the k0kubun:result-with-hash branch from caadf1b to ed1c1e5 May 19, 2017

@k0kubun

This comment has been minimized.

Copy link
Member Author

commented May 19, 2017

I wrote my feeling for this on https://bugs.ruby-lang.org/issues/8631 and want your comments.

@k0kubun

This comment has been minimized.

Copy link
Member Author

commented May 19, 2017

Closing in favor of #1623.

@k0kubun k0kubun closed this May 19, 2017

@k0kubun k0kubun deleted the k0kubun:result-with-hash branch May 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.