Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 42 additions & 12 deletions lib/webmachine/dispatcher/route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,54 @@ class Route
# used to define this route (see #initialize).
attr_reader :path_spec

# @return [Array<Proc>] the list of guard blocks used to define this
# route (see #initialize).
attr_reader :guards

# When used in a path specification, will match all remaining
# segments
MATCH_ALL = '*'.freeze

# Creates a new Route that will associate a pattern to a
# {Resource}.
# @param [Array<String|Symbol>] path_spec a list of path
# segments (String) and identifiers (Symbol) to bind.
# Strings will be simply matched for equality. Symbols in
# the path spec will be extracted into {Request#path_info} for use
# inside your {Resource}. The special segment {MATCH_ALL} will match
# all remaining segments.
# @param [Class] resource the {Resource} to dispatch to
# @param [Hash] bindings additional information to add to
# {Request#path_info} when this route matches
#
# @example Standard route
# Route.new(["*"], MyResource)
#
# @example Guarded route
# Route.new ["/notes"],
# ->(request) { request.method == "POST" },
# Resources::Note
# Route.new ["/notes"], Resources::NoteList
# Route.new ["/notes", :id], Resources::Note
#
# @overload initialize(path_spec, *guards, resource, bindings = {})
# @param [Array<String|Symbol>] path_spec a list of path
# segments (String) and identifiers (Symbol) to bind.
# Strings will be simply matched for equality. Symbols in
# the path spec will be extracted into {Request#path_info} for use
# inside your {Resource}. The special segment {MATCH_ALL} will match
# all remaining segments.
# @param [Proc] guards optional guard blocks called with the request.
# @param [Class] resource the {Resource} to dispatch to
# @param [Hash] bindings additional information to add to
# {Request#path_info} when this route matches
# @see Dispatcher#add_route
def initialize(path_spec, resource, bindings={})
@path_spec, @resource, @bindings = path_spec, resource, bindings
def initialize(path_spec, *args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally not a fan of slurping args in an internal interface like this. I would propose either 1) use a signature that can work without the slurp in the public API (eg., put resource before guards and require an empty list be passed for guards if binding is provided: def initialize(path_spec, resource, guards, bindings = {})), or 2) implement #initialize as described in (1) and support the flexible (slurp-based) public API via a factory method that encapsulates this args processing and not burden #initialize with this logic (eg., def self.build(path_spec, *args)... new(path_spec, resource, guards, bindings)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that can reduce the complexity here would be helpful. I've done many splat-argument methods and they are difficult to maintain and understand. Definitely one place where I wish for pattern-matched function headers like Erlang has!

if args.last.is_a? Hash
bindings = args.pop
else
bindings = {}
end

resource = args.pop
guards = args

@path_spec = path_spec
@guards = guards
@resource = resource
@bindings = bindings

raise ArgumentError, t('not_resource_class', :class => resource.name) unless resource < Resource
end

Expand All @@ -40,7 +70,7 @@ def initialize(path_spec, resource, bindings={})
# @param [Reqeust] request the request object
def match?(request)
tokens = request.uri.path.match(/^\/(.*)/)[1].split('/')
bind(tokens, {})
guards.all? { |guard| guard[request] } && bind(tokens, {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would logically reverse this, that is, put the call to bind first. The difference may be miniscule, but I feel like guards are additional conditions on the match, not the primary means of matching.

end

# Decorates the request with information about the dispatch
Expand Down
44 changes: 44 additions & 0 deletions spec/webmachine/dispatcher/route_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,50 @@
it { should_not match_route [] }
it { should_not match_route %w{bar *} }
end

context "with a guard on the request method" do
let(:route) do
described_class.new(
["notes"],
lambda { |request| request.method == "POST" },
resource
)
end

before do
request.uri.path = "/notes"
end

context "when guard returns true" do
before do
request.method.replace "POST"
end

it "returns true" do
route.match?(request).should be_true
end

context "but the path match fails" do
before do
request.uri.path = "/other"
end

it "returns false" do
route.match?(request).should be_false
end
end
end

context "when guard returns false" do
before do
request.method.replace "GET"
end

it "returns false" do
route.match?(request).should be_false
end
end
end
end

context "applying bindings" do
Expand Down