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

Deprecate safe_level of `ERB.new` in Ruby 2.6 #2179

Merged
merged 1 commit into from May 26, 2018

Conversation

Projects
None yet
3 participants
@koic
Copy link
Contributor

koic commented Mar 14, 2018

The interface of ERB.new will change from Ruby 2.6.

Add :trim_mode and :eoutvar keyword arguments to ERB.new.
Now non-keyword arguments other than first one are softly deprecated
and will be removed when Ruby 2.5 becomes EOL. [Feature #14256]

https://github.com/ruby/ruby/blob/2311087b685e8dc0f21f4a89875f25c22f5c39a9/NEWS#stdlib-updates-outstanding-ones-only

The following address is related Ruby's commit.
ruby/ruby@cc777d0

This PR uses ERB.instance_method(:initialize).parameters.assoc(:key) to switch ERB.new interface. Because Padrino supports multiple Ruby versions, it need to use the appropriate interface.

This patch is built into Ruby.
ruby/ruby@3406c5d

@koic koic force-pushed the koic:deprecate_safe_level_of_erb_new_in_ruby_2_6 branch 2 times, most recently from 9bb45ff to e0f4b4e Mar 14, 2018

@koic koic force-pushed the koic:deprecate_safe_level_of_erb_new_in_ruby_2_6 branch from e0f4b4e to 1b5ac97 May 4, 2018

@wikimatze

This comment has been minimized.

Copy link
Member

wikimatze commented May 9, 2018

@padrino/core-members I thing we can merge it.

@ujifgc

This comment has been minimized.

Copy link
Member

ujifgc commented May 9, 2018

This makes me very sad ERB.instance_method(:initialize).parameters.assoc(:key)

@wikimatze

This comment has been minimized.

Copy link
Member

wikimatze commented May 9, 2018

@ujifgc why?

@ujifgc

This comment has been minimized.

Copy link
Member

ujifgc commented May 9, 2018

Mostly because it looks scary.

And if I'm not mistaken it spawns several useless objects for GC to collect every time #prepare is called.

I'd prefer something like this maybe:

if multiple_object_summoning_to_check_features
  def fast_method
    new_fun_usage
  end
else
  def fast_method
    old_sad_usage
  end
end
@wikimatze

This comment has been minimized.

Copy link
Member

wikimatze commented May 9, 2018

Okay, that is an argument, but spreading useless objects slows down the performance. What do you think @koic? Is there a faster way for your patch?

@koic

This comment has been minimized.

Copy link
Contributor Author

koic commented May 10, 2018

Thanks for review. Although it is different from the Ruby upstream, I can change it to approach using RUBY_VERSION >= '2.6' instead of ERB.instance_method(:initialize).parameters.assoc(:key).

Or I will be able to change it by approach which emphasizes performance which showed as an example (in that case there will be code duplication a bit) .
#2179 (comment)

I think both changes will improve performance better than the current implementation.
I'd like to change according to your preference. Thank you.

Deprecate safe_level of `ERB.new` in Ruby 2.6
The interface of `ERB.new` will change from Ruby 2.6.

> Add :trim_mode and :eoutvar keyword arguments to ERB.new.
> Now non-keyword arguments other than first one are softly deprecated
> and will be removed when Ruby 2.5 becomes EOL. [Feature #14256]

https://github.com/ruby/ruby/blob/2311087b685e8dc0f21f4a89875f25c22f5c39a9/NEWS#stdlib-updates-outstanding-ones-only

The following address is related Ruby's commit.
ruby/ruby@cc777d0

This PR uses `ERB.instance_method(:initialize).parameters.assoc(:key)`
to switch `ERB.new` interface. Because Padrino supports multiple
Ruby versions, it need to use the appropriate interface.

This patch is built into Ruby.
ruby/ruby@3406c5d

@koic koic force-pushed the koic:deprecate_safe_level_of_erb_new_in_ruby_2_6 branch from 1b5ac97 to ff748b7 May 22, 2018

@koic

This comment has been minimized.

Copy link
Contributor Author

koic commented May 22, 2018

I changed the implementation of prepare method according to Ruby version. I will change further if there is a problem.

@ujifgc

This comment has been minimized.

Copy link
Member

ujifgc commented May 22, 2018

Okay, I'm fine with the new version.

@ujifgc ujifgc merged commit e94a65f into padrino:master May 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@koic koic deleted the koic:deprecate_safe_level_of_erb_new_in_ruby_2_6 branch May 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment