From 3b908cba28552604f8d85a236f2d1c82f7589d80 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 18:34:21 -0700 Subject: fewer operations on the options hash since we pass `as` down, then we won't have to do an insert / delete dance with the options hash --- actionpack/lib/action_dispatch/routing/mapper.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index cd94f35e8f..e977d769e0 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -63,7 +63,7 @@ module ActionDispatch attr_reader :requirements, :conditions, :defaults attr_reader :to, :default_controller, :default_action, :as, :anchor - def self.build(scope, set, path, options) + def self.build(scope, set, path, as, options) options = scope[:options].merge(options) if scope[:options] options.delete :only @@ -74,10 +74,10 @@ module ActionDispatch defaults = (scope[:defaults] || {}).merge options.delete(:defaults) || {} - new scope, set, path, defaults, options + new scope, set, path, defaults, as, options end - def initialize(scope, set, path, defaults, options) + def initialize(scope, set, path, defaults, as, options) @requirements, @conditions = {}, {} @defaults = defaults @set = set @@ -85,7 +85,7 @@ module ActionDispatch @to = options.delete :to @default_controller = options.delete(:controller) || scope[:controller] @default_action = options.delete(:action) || scope[:action] - @as = options.delete :as + @as = as @anchor = options.delete :anchor formatted = options.delete :format @@ -1544,13 +1544,13 @@ module ActionDispatch action = nil end - if !options.fetch(:as, true) # if it's set to nil or false - options.delete(:as) - else - options[:as] = name_for_action(options[:as], action) - end + as = if !options.fetch(:as, true) # if it's set to nil or false + options.delete(:as) + else + name_for_action(options.delete(:as), action) + end - mapping = Mapping.build(@scope, @set, URI.parser.escape(path), options) + mapping = Mapping.build(@scope, @set, URI.parser.escape(path), as, options) app, conditions, requirements, defaults, as, anchor = mapping.to_route @set.add_route(app, conditions, requirements, defaults, as, anchor) end -- cgit v1.2.3 From 318eea062ec3aa95929cfb516a273e732e22a9d1 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 18:42:26 -0700 Subject: pass consistent parameters to canonical_action? now we only have to look up @scope[:scope_level] once per call to canonical_action? and we don't have a variable named "flag" --- actionpack/lib/action_dispatch/routing/mapper.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index e977d769e0..a3a22bceb1 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1634,8 +1634,8 @@ module ActionDispatch RESOURCE_SCOPES.include? @scope[:scope_level] end - def resource_method_scope? #:nodoc: - RESOURCE_METHOD_SCOPES.include? @scope[:scope_level] + def resource_method_scope?(scope_level) #:nodoc: + RESOURCE_METHOD_SCOPES.include? scope_level end def nested_scope? #:nodoc: @@ -1699,8 +1699,8 @@ module ActionDispatch @scope[:constraints][parent_resource.param] end - def canonical_action?(action, flag) #:nodoc: - flag && resource_method_scope? && CANONICAL_ACTIONS.include?(action.to_s) + def canonical_action?(action, scope_level) #:nodoc: + scope_level && resource_method_scope?(scope_level) && CANONICAL_ACTIONS.include?(action.to_s) end def shallow_scope(path, options = {}) #:nodoc: @@ -1714,7 +1714,7 @@ module ActionDispatch end def path_for_action(action, path) #:nodoc: - if canonical_action?(action, path.blank?) + if path.blank? && canonical_action?(action, @scope[:scope_level]) @scope[:path].to_s else "#{@scope[:path]}/#{action_path(action, path)}" -- cgit v1.2.3 From 91608dc342237372548ccbe403ef06c56c2755f2 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 18:47:16 -0700 Subject: only test `prefix` once we don't need to repeat if statements --- actionpack/lib/action_dispatch/routing/mapper.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index a3a22bceb1..37f781a7dd 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1732,12 +1732,14 @@ module ActionDispatch elsif !canonical_action?(action, @scope[:scope_level]) prefix = action end - prefix.to_s.tr('-', '_') if prefix + + if prefix + Mapper.normalize_name prefix.to_s.tr('-', '_') + end end def name_for_action(as, action) #:nodoc: prefix = prefix_name_for_action(as, action) - prefix = Mapper.normalize_name(prefix) if prefix name_prefix = @scope[:as] if parent_resource -- cgit v1.2.3 From 0127f02826fec6641c21779c4109a1d2ccb700fe Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:13:58 -0700 Subject: only look up scope level once avoid hash lookups and remove depency on the instance --- actionpack/lib/action_dispatch/routing/mapper.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 37f781a7dd..2f03e22be3 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1726,20 +1726,21 @@ module ActionDispatch path || @scope[:path_names][name] || name.to_s end - def prefix_name_for_action(as, action) #:nodoc: + def prefix_name_for_action(as, action, scope_level) #:nodoc: if as prefix = as - elsif !canonical_action?(action, @scope[:scope_level]) + elsif !canonical_action?(action, scope_level) prefix = action end - if prefix + if prefix && prefix != '/' && !prefix.empty? Mapper.normalize_name prefix.to_s.tr('-', '_') end end def name_for_action(as, action) #:nodoc: - prefix = prefix_name_for_action(as, action) + scope_level = @scope[:scope_level] + prefix = prefix_name_for_action(as, action, scope_level) name_prefix = @scope[:as] if parent_resource @@ -1749,7 +1750,7 @@ module ActionDispatch member_name = parent_resource.member_name end - name = case @scope[:scope_level] + name = case scope_level when :nested [name_prefix, prefix] when :collection @@ -1764,7 +1765,7 @@ module ActionDispatch [name_prefix, member_name, prefix] end - if candidate = name.select(&:present?).join("_").presence + if candidate = name.compact.join("_").presence # If a name was not explicitly given, we check if it is valid # and return nil in case it isn't. Otherwise, we pass the invalid name # forward so the underlying router engine treats it and raises an exception. -- cgit v1.2.3 From 911ef972a540686fd7fe7a3dee659ce3bb1fd12d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:22:30 -0700 Subject: move scope_level to a method on the scope object now we don't have to have a hard coded key --- actionpack/lib/action_dispatch/routing/mapper.rb | 27 ++++++++++++++++++------ 1 file changed, 20 insertions(+), 7 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 2f03e22be3..77d9889f28 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1047,7 +1047,6 @@ module ActionDispatch RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except, :param, :concerns] CANONICAL_ACTIONS = %w(index create new show update destroy) RESOURCE_METHOD_SCOPES = [:collection, :member, :new] - RESOURCE_SCOPES = [:resource, :resources] class Resource #:nodoc: attr_reader :controller, :path, :options, :param @@ -1521,7 +1520,7 @@ module ActionDispatch if on = options.delete(:on) send(on) { decomposed_match(path, options) } else - case @scope[:scope_level] + case @scope.scope_level when :resources nested { decomposed_match(path, options) } when :resource @@ -1564,7 +1563,7 @@ module ActionDispatch raise ArgumentError, "must be called with a path and/or options" end - if @scope[:scope_level] == :resources + if @scope.scope_level == :resources with_scope_level(:root) do scope(parent_resource.path) do super(options) @@ -1631,7 +1630,7 @@ module ActionDispatch end def resource_scope? #:nodoc: - RESOURCE_SCOPES.include? @scope[:scope_level] + @scope.resource_scope? end def resource_method_scope?(scope_level) #:nodoc: @@ -1639,7 +1638,7 @@ module ActionDispatch end def nested_scope? #:nodoc: - @scope[:scope_level] == :nested + @scope.nested? end def with_exclusive_scope @@ -1714,7 +1713,7 @@ module ActionDispatch end def path_for_action(action, path) #:nodoc: - if path.blank? && canonical_action?(action, @scope[:scope_level]) + if path.blank? && canonical_action?(action, @scope.scope_level) @scope[:path].to_s else "#{@scope[:path]}/#{action_path(action, path)}" @@ -1739,7 +1738,7 @@ module ActionDispatch end def name_for_action(as, action) #:nodoc: - scope_level = @scope[:scope_level] + scope_level = @scope.scope_level prefix = prefix_name_for_action(as, action, scope_level) name_prefix = @scope[:as] @@ -1900,6 +1899,8 @@ module ActionDispatch :controller, :action, :path_names, :constraints, :shallow, :blocks, :defaults, :options] + RESOURCE_SCOPES = [:resource, :resources] + attr_reader :parent def initialize(hash, parent = {}) @@ -1907,6 +1908,18 @@ module ActionDispatch @parent = parent end + def scope_level + self[:scope_level] + end + + def nested? + scope_level == :nested + end + + def resource_scope? + RESOURCE_SCOPES.include? scope_level + end + def options OPTIONS end -- cgit v1.2.3 From 19bb6770c0b4a7970a8c6daa2a1ed0f926e721f2 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:24:40 -0700 Subject: move the scope level key fully inside the scope object --- actionpack/lib/action_dispatch/routing/mapper.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 77d9889f28..9c9c56fbce 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1654,7 +1654,7 @@ module ActionDispatch end def with_scope_level(kind) - @scope = @scope.new(:scope_level => kind) + @scope = @scope.new_level(kind) yield ensure @scope = @scope.parent @@ -1928,6 +1928,10 @@ module ActionDispatch self.class.new hash, self end + def new_level(level) + new(:scope_level => level) + end + def [](key) @hash.fetch(key) { @parent[key] } end -- cgit v1.2.3 From 677bc212eb7f35ad0f8808f56450a4b6b2340023 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:29:24 -0700 Subject: scope_level is no longer a hash key, just use the ivar --- actionpack/lib/action_dispatch/routing/mapper.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 9c9c56fbce..fa273ac201 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1903,13 +1903,14 @@ module ActionDispatch attr_reader :parent - def initialize(hash, parent = {}) + def initialize(hash, parent = {}, scope_level = nil) @hash = hash @parent = parent + @scope_level = scope_level end def scope_level - self[:scope_level] + @scope_level end def nested? @@ -1925,11 +1926,15 @@ module ActionDispatch end def new(hash) - self.class.new hash, self + self.class.new hash, self, scope_level end def new_level(level) - new(:scope_level => level) + self.class.new(self, self, level) + end + + def fetch(key, &block) + @hash.fetch(key, &block) end def [](key) -- cgit v1.2.3 From 047af8dd3c304fbfe625be8b7c600005c69fa593 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:29:39 -0700 Subject: change to attr_reader --- actionpack/lib/action_dispatch/routing/mapper.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index fa273ac201..209af7a16d 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1901,7 +1901,7 @@ module ActionDispatch RESOURCE_SCOPES = [:resource, :resources] - attr_reader :parent + attr_reader :parent, :scope_level def initialize(hash, parent = {}, scope_level = nil) @hash = hash @@ -1909,10 +1909,6 @@ module ActionDispatch @scope_level = scope_level end - def scope_level - @scope_level - end - def nested? scope_level == :nested end -- cgit v1.2.3 From 374d66be3e9e994bbe8ad2702ee2209c24b581b0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:40:57 -0700 Subject: reduce calls to scope_level this will help us to encapsulate magical symbols so hopefully we can eliminate hardcoded magic symbols --- actionpack/lib/action_dispatch/routing/mapper.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 209af7a16d..46d83f7fd7 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1563,7 +1563,7 @@ module ActionDispatch raise ArgumentError, "must be called with a path and/or options" end - if @scope.scope_level == :resources + if @scope.resources? with_scope_level(:root) do scope(parent_resource.path) do super(options) @@ -1913,6 +1913,10 @@ module ActionDispatch scope_level == :nested end + def resources? + scope_level == :resources + end + def resource_scope? RESOURCE_SCOPES.include? scope_level end -- cgit v1.2.3 From e4cb3819dfe36cc9a8396fb207b74980d7bd0cd5 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:44:05 -0700 Subject: ask the scope for the action name --- actionpack/lib/action_dispatch/routing/mapper.rb | 32 +++++++++++++----------- 1 file changed, 18 insertions(+), 14 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 46d83f7fd7..1a7e9d32e6 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1749,20 +1749,7 @@ module ActionDispatch member_name = parent_resource.member_name end - name = case scope_level - when :nested - [name_prefix, prefix] - when :collection - [prefix, name_prefix, collection_name] - when :new - [prefix, :new, name_prefix, member_name] - when :member - [prefix, name_prefix, member_name] - when :root - [name_prefix, collection_name, prefix] - else - [name_prefix, member_name, prefix] - end + name = @scope.action_name(name_prefix, prefix, collection_name, member_name) if candidate = name.compact.join("_").presence # If a name was not explicitly given, we check if it is valid @@ -1917,6 +1904,23 @@ module ActionDispatch scope_level == :resources end + def action_name(name_prefix, prefix, collection_name, member_name) + case scope_level + when :nested + [name_prefix, prefix] + when :collection + [prefix, name_prefix, collection_name] + when :new + [prefix, :new, name_prefix, member_name] + when :member + [prefix, name_prefix, member_name] + when :root + [name_prefix, collection_name, prefix] + else + [name_prefix, member_name, prefix] + end + end + def resource_scope? RESOURCE_SCOPES.include? scope_level end -- cgit v1.2.3 From 43ce6e22b19245318ff154f859c82272130ac238 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 13 Aug 2014 19:47:26 -0700 Subject: ask the scope object if it is a resource_method_scope --- actionpack/lib/action_dispatch/routing/mapper.rb | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) (limited to 'actionpack/lib/action_dispatch/routing/mapper.rb') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 1a7e9d32e6..e92baa5aa7 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1046,7 +1046,6 @@ module ActionDispatch VALID_ON_OPTIONS = [:new, :collection, :member] RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except, :param, :concerns] CANONICAL_ACTIONS = %w(index create new show update destroy) - RESOURCE_METHOD_SCOPES = [:collection, :member, :new] class Resource #:nodoc: attr_reader :controller, :path, :options, :param @@ -1633,8 +1632,8 @@ module ActionDispatch @scope.resource_scope? end - def resource_method_scope?(scope_level) #:nodoc: - RESOURCE_METHOD_SCOPES.include? scope_level + def resource_method_scope? #:nodoc: + @scope.resource_method_scope? end def nested_scope? #:nodoc: @@ -1698,8 +1697,8 @@ module ActionDispatch @scope[:constraints][parent_resource.param] end - def canonical_action?(action, scope_level) #:nodoc: - scope_level && resource_method_scope?(scope_level) && CANONICAL_ACTIONS.include?(action.to_s) + def canonical_action?(action) #:nodoc: + resource_method_scope? && CANONICAL_ACTIONS.include?(action.to_s) end def shallow_scope(path, options = {}) #:nodoc: @@ -1713,7 +1712,7 @@ module ActionDispatch end def path_for_action(action, path) #:nodoc: - if path.blank? && canonical_action?(action, @scope.scope_level) + if path.blank? && canonical_action?(action) @scope[:path].to_s else "#{@scope[:path]}/#{action_path(action, path)}" @@ -1725,10 +1724,10 @@ module ActionDispatch path || @scope[:path_names][name] || name.to_s end - def prefix_name_for_action(as, action, scope_level) #:nodoc: + def prefix_name_for_action(as, action) #:nodoc: if as prefix = as - elsif !canonical_action?(action, scope_level) + elsif !canonical_action?(action) prefix = action end @@ -1738,8 +1737,7 @@ module ActionDispatch end def name_for_action(as, action) #:nodoc: - scope_level = @scope.scope_level - prefix = prefix_name_for_action(as, action, scope_level) + prefix = prefix_name_for_action(as, action) name_prefix = @scope[:as] if parent_resource @@ -1887,6 +1885,7 @@ module ActionDispatch :shallow, :blocks, :defaults, :options] RESOURCE_SCOPES = [:resource, :resources] + RESOURCE_METHOD_SCOPES = [:collection, :member, :new] attr_reader :parent, :scope_level @@ -1904,6 +1903,10 @@ module ActionDispatch scope_level == :resources end + def resource_method_scope? + RESOURCE_METHOD_SCOPES.include? scope_level + end + def action_name(name_prefix, prefix, collection_name, member_name) case scope_level when :nested -- cgit v1.2.3