Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Add has_named_route? to the mapper API
  • Loading branch information
José Valim committed May 20, 2013
1 parent ef62a51 commit 7a993ff
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 0 deletions.
4 changes: 4 additions & 0 deletions actionpack/CHANGELOG.md
@@ -1,5 +1,9 @@
## unreleased ##

* Add `has_named_route?(route_name)` to the mapper API.

*José Valim*

* Fix an issue where partials with a number in the filename weren't being digested for cache dependencies.

*Bryan Ricker*
Expand Down
5 changes: 5 additions & 0 deletions actionpack/lib/action_dispatch/routing/mapper.rb
Expand Up @@ -515,6 +515,11 @@ def with_default_scope(scope, &block)
end
end

# Query if the following named route was already defined.
def has_named_route?(name)
@set.named_routes.routes[name.to_sym]

This comment has been minimized.

Copy link
@ericboehs

ericboehs Aug 26, 2013

Shouldn't you do

!!@set.named_routes.routes[name.to_sym]

So that it returns false rather than nil?

Or better yet

@set.named_routes.routes.include?(name.to_sym)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Aug 27, 2013

Member

nil is also a falsy value. Query methods doesn't need to return true or false in ruby.

This comment has been minimized.

Copy link
@ericboehs

ericboehs Aug 27, 2013

I know nil is falsey hence the !!. I don't think it's a good practice to return nil when you mean false though. No other Rails' or ActiveSupport query methods return nil that I know of.

Also include? reads more like how you'd explain it to the reader.

This comment has been minimized.

Copy link
@bradleypriest

bradleypriest Aug 27, 2013

Contributor

@ericboehs see #5329 for some previous extreme bikeshedding on predicates

This comment has been minimized.

Copy link
@ericboehs

ericboehs Aug 27, 2013

Dear God, it's a mad house in there.

Turn back now!

end

private
def app_name(app)
return unless app.respond_to?(:routes)
Expand Down
13 changes: 13 additions & 0 deletions actionpack/test/dispatch/routing_test.rb
Expand Up @@ -2662,6 +2662,19 @@ def test_redirect_argument_error
assert_raises(ArgumentError) { routes.redirect Object.new }
end

def test_named_route_check
before, after = nil

draw do
before = has_named_route?(:hello)
get "/hello", as: :hello, to: "hello#world"
after = has_named_route?(:hello)
end

assert !before, "expected to not have named route :hello before route definition"
assert after, "expected to have named route :hello after route definition"
end

def test_explicitly_avoiding_the_named_route
draw do
scope :as => "routes" do
Expand Down

0 comments on commit 7a993ff

Please sign in to comment.