From f875331e3282228d2019392b1653fb8ea39fd711 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 16 Jul 2014 17:27:03 -0700 Subject: only extract :params from the options hash once --- actionpack/lib/action_dispatch/http/url.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index 6ba2820d09..f5e71f5a3d 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -48,9 +48,8 @@ module ActionDispatch end if options.key? :params - params = options[:params].is_a?(Hash) ? - options[:params] : - { params: options[:params] } + param = options[:params] + params = param.is_a?(Hash) ? param : { params: param } params.reject! { |_,v| v.to_param.nil? } result << "?#{params.to_query}" unless params.empty? -- cgit v1.2.3 From 69799eda94c41461cf4a8157f457f0be4a012611 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 16 Jul 2014 18:13:22 -0700 Subject: break out path building logic to methods --- actionpack/lib/action_dispatch/http/url.rb | 36 ++++++++++++++++++------------ 1 file changed, 22 insertions(+), 14 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index f5e71f5a3d..fc1d0e6663 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -34,19 +34,26 @@ module ActionDispatch raise ArgumentError, 'Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true' end - path = options[:script_name].to_s.chomp("/") - path << options[:path].to_s + result = options[:script_name].to_s.chomp("/") + result << options[:path].to_s - path = add_trailing_slash(path) if options[:trailing_slash] + result = add_trailing_slash(result) if options[:trailing_slash] - result = if options[:only_path] - path - else - protocol = options[:protocol] - port = options[:port] - build_host_url(host, port, protocol, options).concat path - end + result = add_params options, result + result = add_anchor options, result + if options[:only_path] + result + else + protocol = options[:protocol] + port = options[:port] + build_host_url(host, port, protocol, options, result) + end + end + + private + + def add_params(options, result) if options.key? :params param = options[:params] params = param.is_a?(Hash) ? param : { params: param } @@ -54,13 +61,14 @@ module ActionDispatch params.reject! { |_,v| v.to_param.nil? } result << "?#{params.to_query}" unless params.empty? end + result + end + def add_anchor(options, result) result << "##{Journey::Router::Utils.escape_fragment(options[:anchor].to_param.to_s)}" if options[:anchor] result end - private - def extract_domain_from(host, tld_length) host.split('.').last(1 + tld_length).join('.') end @@ -82,7 +90,7 @@ module ActionDispatch path end - def build_host_url(host, port, protocol, options) + def build_host_url(host, port, protocol, options, path) if match = host.match(HOST_REGEXP) protocol ||= match[1] unless protocol == false host = match[2] @@ -103,7 +111,7 @@ module ActionDispatch result << ":#{normalized_port}" } - result + result.concat path end def named_host?(host) -- cgit v1.2.3 From 0e26271456a772de5ce4c4c78cce3a275bcb89a8 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 16 Jul 2014 18:15:15 -0700 Subject: extract path building to a method --- actionpack/lib/action_dispatch/http/url.rb | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index fc1d0e6663..5decab3d8f 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -34,21 +34,23 @@ module ActionDispatch raise ArgumentError, 'Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true' end + if options[:only_path] + path_for options + else + protocol = options[:protocol] + port = options[:port] + build_host_url(host, port, protocol, options, path_for(options)) + end + end + + def path_for(options) result = options[:script_name].to_s.chomp("/") result << options[:path].to_s result = add_trailing_slash(result) if options[:trailing_slash] result = add_params options, result - result = add_anchor options, result - - if options[:only_path] - result - else - protocol = options[:protocol] - port = options[:port] - build_host_url(host, port, protocol, options, result) - end + add_anchor options, result end private -- cgit v1.2.3 From 2888f8653e2e0a6394e41cb4e8db2e2d81313eb7 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 17 Jul 2014 10:47:58 -0700 Subject: use a strategy object for generating urls in named helpers since we know that the route should be a path or fully qualified, we can pass a strategy object that handles generation. This allows us to eliminate an "if only_path" branch when generating urls. --- actionpack/lib/action_dispatch/http/url.rb | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index 5decab3d8f..473f692b05 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -29,20 +29,25 @@ module ActionDispatch end def url_for(options) - host = options[:host] - unless host || options[:only_path] - raise ArgumentError, 'Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true' - end - if options[:only_path] path_for options else - protocol = options[:protocol] - port = options[:port] - build_host_url(host, port, protocol, options, path_for(options)) + full_url_for options end end + def full_url_for(options) + host = options[:host] + protocol = options[:protocol] + port = options[:port] + + unless host + raise ArgumentError, 'Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true' + end + + build_host_url(host, port, protocol, options, path_for(options)) + end + def path_for(options) result = options[:script_name].to_s.chomp("/") result << options[:path].to_s -- cgit v1.2.3 From 8d61463f34fd0bb7bad446f2d43aaa63fee61563 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Thu, 31 Jul 2014 13:05:56 -0300 Subject: Push options check up so we can simplify internal methods --- actionpack/lib/action_dispatch/http/url.rb | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index 473f692b05..32234d18ab 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -51,28 +51,25 @@ module ActionDispatch def path_for(options) result = options[:script_name].to_s.chomp("/") result << options[:path].to_s - result = add_trailing_slash(result) if options[:trailing_slash] - - result = add_params options, result - add_anchor options, result + result = add_params(result, options[:params]) if options.key?(:params) + result = add_anchor(result, options[:anchor]) if options.key?(:anchor) + result end private - def add_params(options, result) - if options.key? :params - param = options[:params] - params = param.is_a?(Hash) ? param : { params: param } + def add_params(result, param) + params = param.is_a?(Hash) ? param : { params: param } + + params.reject! { |_,v| v.to_param.nil? } + result << "?#{params.to_query}" unless params.empty? - params.reject! { |_,v| v.to_param.nil? } - result << "?#{params.to_query}" unless params.empty? - end result end - def add_anchor(options, result) - result << "##{Journey::Router::Utils.escape_fragment(options[:anchor].to_param.to_s)}" if options[:anchor] + def add_anchor(result, anchor) + result << "##{Journey::Router::Utils.escape_fragment(anchor.to_param.to_s)}" result end -- cgit v1.2.3 From 277247110cb77e69ea113a3aa77a9c12322ffcfb Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Thu, 31 Jul 2014 13:08:58 -0300 Subject: Simplify conditional --- actionpack/lib/action_dispatch/http/url.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index 32234d18ab..12d3b10981 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -59,9 +59,8 @@ module ActionDispatch private - def add_params(result, param) - params = param.is_a?(Hash) ? param : { params: param } - + def add_params(result, params) + params = { params: params } unless params.is_a?(Hash) params.reject! { |_,v| v.to_param.nil? } result << "?#{params.to_query}" unless params.empty? -- cgit v1.2.3 From fafff357cc775dab2cc46feaf43d311a7a042283 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Thu, 31 Jul 2014 13:09:32 -0300 Subject: Rename variable to better show its intent --- actionpack/lib/action_dispatch/http/url.rb | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index 12d3b10981..ef07a101ad 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -49,27 +49,27 @@ module ActionDispatch end def path_for(options) - result = options[:script_name].to_s.chomp("/") - result << options[:path].to_s - result = add_trailing_slash(result) if options[:trailing_slash] - result = add_params(result, options[:params]) if options.key?(:params) - result = add_anchor(result, options[:anchor]) if options.key?(:anchor) - result + path = options[:script_name].to_s.chomp("/") + path << options[:path].to_s + path = add_trailing_slash(path) if options[:trailing_slash] + path = add_params(path, options[:params]) if options.key?(:params) + path = add_anchor(path, options[:anchor]) if options.key?(:anchor) + path end private - def add_params(result, params) + def add_params(path, params) params = { params: params } unless params.is_a?(Hash) params.reject! { |_,v| v.to_param.nil? } - result << "?#{params.to_query}" unless params.empty? + path << "?#{params.to_query}" unless params.empty? - result + path end - def add_anchor(result, anchor) - result << "##{Journey::Router::Utils.escape_fragment(anchor.to_param.to_s)}" - result + def add_anchor(path, anchor) + path << "##{Journey::Router::Utils.escape_fragment(anchor.to_param.to_s)}" + path end def extract_domain_from(host, tld_length) -- cgit v1.2.3 From 091a59301f65601b47c7022ac5028b7f6b569e00 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Thu, 31 Jul 2014 13:12:04 -0300 Subject: Only concatenate path if it was given rather than converting blindly --- actionpack/lib/action_dispatch/http/url.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index ef07a101ad..55baba88e4 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -50,7 +50,7 @@ module ActionDispatch def path_for(options) path = options[:script_name].to_s.chomp("/") - path << options[:path].to_s + path << options[:path] if options.key?(:path) path = add_trailing_slash(path) if options[:trailing_slash] path = add_params(path, options[:params]) if options.key?(:params) path = add_anchor(path, options[:anchor]) if options.key?(:anchor) -- cgit v1.2.3 From 0b859dfefe4f267eb86fa6194fa556caae5cb44c Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Thu, 31 Jul 2014 13:14:14 -0300 Subject: Do not reassign variable when mutation is happening These methods mutate the path variable/argument so there is no need to reassign it every time. --- actionpack/lib/action_dispatch/http/url.rb | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index 55baba88e4..a7dbceed67 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -51,9 +51,11 @@ module ActionDispatch def path_for(options) path = options[:script_name].to_s.chomp("/") path << options[:path] if options.key?(:path) - path = add_trailing_slash(path) if options[:trailing_slash] - path = add_params(path, options[:params]) if options.key?(:params) - path = add_anchor(path, options[:anchor]) if options.key?(:anchor) + + add_trailing_slash(path) if options[:trailing_slash] + add_params(path, options[:params]) if options.key?(:params) + add_anchor(path, options[:anchor]) if options.key?(:anchor) + path end @@ -63,13 +65,10 @@ module ActionDispatch params = { params: params } unless params.is_a?(Hash) params.reject! { |_,v| v.to_param.nil? } path << "?#{params.to_query}" unless params.empty? - - path end def add_anchor(path, anchor) path << "##{Journey::Router::Utils.escape_fragment(anchor.to_param.to_s)}" - path end def extract_domain_from(host, tld_length) @@ -89,8 +88,6 @@ module ActionDispatch elsif !path.include?(".") path.sub!(/[^\/]\z|\A\z/, '\&/') end - - path end def build_host_url(host, port, protocol, options, path) -- cgit v1.2.3 From ddb0d4bec16aea562dd155577d891e83fa066410 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Thu, 31 Jul 2014 13:18:51 -0300 Subject: Realign assignments :scissors: --- actionpack/lib/action_dispatch/http/url.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'actionpack/lib/action_dispatch/http/url.rb') diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index a7dbceed67..6b8dcaf497 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -92,13 +92,13 @@ module ActionDispatch def build_host_url(host, port, protocol, options, path) if match = host.match(HOST_REGEXP) - protocol ||= match[1] unless protocol == false - host = match[2] - port = match[3] unless options.key? :port + protocol ||= match[1] unless protocol == false + host = match[2] + port = match[3] unless options.key? :port end - protocol = normalize_protocol protocol - host = normalize_host(host, options) + protocol = normalize_protocol protocol + host = normalize_host(host, options) result = protocol.dup -- cgit v1.2.3