mo fasta and mo betta the url_for #5750

Merged
merged 2 commits into from Apr 5, 2012
Jump to file or symbol
Failed to load files and symbols.
+22 −2
Split
@@ -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

rafaelfranca Apr 5, 2012

Owner

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 Apr 5, 2012

Contributor

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 Apr 5, 2012

Contributor

@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"

+ sym = k.to_sym
+ symbolized[sym] = options[k] unless symbolized.has_key?(sym)
+ end
+ url_options.keys.each do |k|
+ sym = k.to_sym
+ symbolized[sym] = url_options[k] unless symbolized.has_key?(sym)
+ end
+ _routes.url_for(symbolized)
when String
options
- when nil, Hash
- _routes.url_for((options || {}).symbolize_keys.reverse_merge!(url_options))
else
polymorphic_url(options)
end
@@ -356,6 +356,15 @@ def test_with_hash_with_indifferent_access
assert_equal("/c/a", W.new.url_for(HashWithIndifferentAccess.new('controller' => 'c', 'action' => 'a', 'only_path' => true)))
end
+ def test_with_stringified_default_url_options
+ W.default_url_options['controller'] = 'd'
+ W.default_url_options['only_path'] = false
+ assert_equal("/c", W.new.url_for(:controller => 'c', :only_path => true))
+
+ W.default_url_options['action'] = 'b'
+ assert_equal("/c/a", W.new.url_for(:controller => 'c', :action => 'a', :only_path => true))
+ end
+
def test_url_params_with_nil_to_param_are_not_in_url
assert_equal("/c/a", W.new.url_for(:only_path => true, :controller => 'c', :action => 'a', :id => Struct.new(:to_param).new(nil)))
end