Permalink
Browse files

Revert "Rewrite journey routes formatter for performance"

This reverts commit 5c224de.

Conflicts:
	actionpack/lib/action_dispatch/journey/visitors.rb

5c224de introduced a bug in the
formatter.  This commit includes a regression test.
  • Loading branch information...
1 parent 03035d6 commit 62d1b330c408813f8115d9e40e24f7801a718444 @tenderlove tenderlove committed May 19, 2014
Showing with 43 additions and 28 deletions.
  1. +23 −28 actionpack/lib/action_dispatch/journey/visitors.rb
  2. +20 −0 actionpack/test/controller/url_for_test.rb
@@ -107,10 +107,11 @@ def visit_GROUP(node)
# Used for formatting urls (url_for)
class Formatter < Visitor # :nodoc:
- attr_reader :options
+ attr_reader :options, :consumed
def initialize(options)
@options = options
+ @consumed = {}
end
private
@@ -122,41 +123,35 @@ def escape_segment(value)
Router::Utils.escape_segment(value)
end
- def visit(node, optional = false)
- case node.type
- when :LITERAL, :SLASH, :DOT
- node.left
- when :STAR
- visit_STAR(node.left)
- when :GROUP
- visit(node.left, true)
- when :CAT
- visit_CAT(node, optional)
- when :SYMBOL
- visit_SYMBOL(node, node.to_sym)
+ def visit_GROUP(node)
+ if consumed == options
+ nil
+ else
+ route = visit(node.left)
+ route.include?("\0") ? nil : route
end
end
- def visit_CAT(node, optional)
- left = visit(node.left, optional)
- right = visit(node.right, optional)
+ def terminal(node)
+ node.left
+ end
- if optional && !(right && left)
- ""
- else
- [left, right].join
- end
+ def binary(node)
+ [visit(node.left), visit(node.right)].join
end
- def visit_STAR(node)
- if value = options[node.to_sym]
- escape_path(value)
- end
+ def nary(node)
+ node.children.map { |c| visit(c) }.join
end
- def visit_SYMBOL(node, name)
- if value = options[name]
- name == :controller ? escape_path(value) : escape_segment(value)
+ def visit_SYMBOL(node)
+ key = node.to_sym
+
+ if value = options[key]
+ consumed[key] = value
+ Router::Utils.escape_path(value)
+ else
+ "\0"
end
end
end
@@ -11,6 +11,26 @@ def teardown
W.default_url_options.clear
end
+ def test_nested_optional
+ klass = Class.new {
+ include ActionDispatch::Routing::RouteSet.new.tap { |r|
+ r.draw {
+ get "/foo/(:bar/(:baz))/:zot", :as => 'fun',
+ :controller => :articles,
+ :action => :index
+ }
+ }.url_helpers
+ self.default_url_options[:host] = 'example.com'
+ }
+
+ path = klass.new.fun_path({:controller => :articles,
+ :baz => "baz",
+ :zot => "zot",
+ :only_path => true })
+ # :bar key isn't provided
+ assert_equal '/foo/zot', path
+ end
+
def add_host!
W.default_url_options[:host] = 'www.basecamphq.com'
end

0 comments on commit 62d1b33

Please sign in to comment.