mo fasta and mo betta the url_for #5750

Merged
merged 2 commits into from Apr 5, 2012

4 participants

@ahoward

incorporated @tenderlove's suggestions.

@carlosantoniodasilva
Ruby on Rails member

@ahoward great! Just a note, it's always good if you can link to the other related issue when sending a new issue / pull request, so others who didn't see it before can follow what's going on. Thanks :)

Update: related to #5743.

@rafaelfranca rafaelfranca commented on the diff Apr 5, 2012
actionpack/lib/action_dispatch/routing/url_for.rb
@@ -144,10 +144,21 @@ def url_options
# # => 'http://somehost.org/tasks/testing?number=33'
def url_for(options = nil)
case options
+ when nil
+ _routes.url_for(url_options)
+ when Hash
+ symbolized = {}
+ options.keys.each do |k|
@rafaelfranca
Ruby on Rails member

Can we create a private method to do this?

Something like this:

def merge_hash(target, base)
  base.keys.each do |k|
    sym = k.to_sym
    target[sym] = base[k] unless target.has_key?(sym)
  end

  target
end

and call it like this:

symbolized = {}
merge_hash(symbolized, options)
merge_hash(symbolized, url_options)
_routes.url_for(symbolized)

What did you think?

@ahoward
ahoward added a note Apr 5, 2012

i did, but if you read the previous discussion people are worried about 1.0 vs 0.4 seconds for 1_000_000 iterations. adding a function call won't help you there. personally i would, but it seems contrary to the goal of making it as fast as possible. again, i'd personally go for well factored code and let the byte code writers get busy.... /cc @tenderlove

@ahoward
ahoward added a note Apr 5, 2012

@rafaelfranca this also makes the same bad assumption the current does: that target is symbolized. the bugz being caused can be summarized as

"we can't know if options or url_options has had string or symbol keys written into it"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove merged commit 174cf8b into rails:master Apr 5, 2012
@spastorino spastorino added a commit that referenced this pull request Apr 9, 2012
@spastorino spastorino Revert "Merge pull request #5750 from ahoward/master"
This reverts commit 174cf8b, reversing
changes made to 7ecd6a7.
The reverted commit improved the performance in the wrong place, now we
have added this 6ddbd18 improvement.
500c9a1
@romanvbabenko romanvbabenko added a commit to romanvbabenko/rails that referenced this pull request May 2, 2012
@spastorino spastorino Revert "Merge pull request #5750 from ahoward/master"
This reverts commit 174cf8b, reversing
changes made to 7ecd6a7.
The reverted commit improved the performance in the wrong place, now we
have added this 6ddbd18 improvement.
6126078
@korny

Very nice!

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