Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Move symbolize keys to the inner options as we can assume url_options…

… will be properly symbolized.
  • Loading branch information...
commit c41f08cefe6fa3747ee79001d9c88dc988e8064d 1 parent 5da90b3
@josevalim josevalim authored
View
2  actionpack/lib/action_dispatch/routing/route_set.rb
@@ -557,7 +557,7 @@ def url_for(options)
path_addition, params = generate(path_options, path_segments || {})
path << path_addition
- ActionDispatch::Http::URL.url_for(options.merge({
+ ActionDispatch::Http::URL.url_for(options.merge!({
:path => path,
:params => params,
:user => user,
View
2  actionpack/lib/action_dispatch/routing/url_for.rb
@@ -145,7 +145,7 @@ def url_for(options = nil)
when String
options
when nil, Hash
- _routes.url_for((options || {}).reverse_merge(url_options).symbolize_keys)
+ _routes.url_for((options || {}).symbolize_keys.reverse_merge!(url_options))
@sobrinho
sobrinho added a note

It's more faster using this way:

_routes.url_for((options.try(:symbolize_keys) || {}).reverse_merge!(url_options))

My benchmark:

require 'benchmark'
require 'active_support/core_ext/hash/keys'
require 'active_support/core_ext/object/try'

options = nil

puts Benchmark.realtime { 1_000_000.times { (options || {}).symbolize_keys } }
puts Benchmark.realtime { 1_000_000.times { (options.try(:symbolize_keys) || {}) } }
1.030134
0.417733

Could you check that @josevalim and @tenderlove?

Obviously this will be better if options is normally nil but I guess this is the case in many situations.

@sobrinho
sobrinho added a note

In the case of options is present, this will slow down a little bit:

require 'benchmark'
require 'active_support/core_ext/hash/keys'
require 'active_support/core_ext/object/try'

options = {}

puts Benchmark.realtime { 1_000_000.times { (options || {}).symbolize_keys } }
puts Benchmark.realtime { 1_000_000.times { (options.try(:symbolize_keys) || {}) } }
0.923583
1.240442
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
else
polymorphic_url(options)
end

7 comments on commit c41f08c

@ahoward

i just hit an issue with this code today - ahoward/rails_default_url_options#1 - and also - customink/central_logger#28

it's faster, but it breaks the interface completely: passing HashWithIndifferntAcess or a Map will break badly since the target of symbolize_keys is reversed ;-/

@ahoward

this is correct, and faster: https://gist.github.com/2305719

@josevalim
Owner
@ahoward

will do: if you like the approach? ker-splatting that code is fugly - but fast and right too...

@ahoward

done -> #5743

@ahoward

updated, and GTG -> #5750

@ahoward

this is both concise, fast, and correct: https://gist.github.com/2305719

Please sign in to comment.
Something went wrong with that request. Please try again.