fix routing issue #92
Conversation
|
Please update the integration test apps with a Rails engine and write a test verifying these changes work. |
ipmsteven
force-pushed
the
fixRouter2
branch
5 times, most recently
from
cfa630d to
c49737a
Compare
|
@dtognazzini which rails version we should support? all rails 3.x + rails 4? maybe drop support for rails versions lower then 3.1 vlado/rails_db_info#8 |
ipmsteven
force-pushed
the
fixRouter2
branch
2 times, most recently
from
e930f30 to
b5a357a
Compare
|
This is great! To answer your question about support: I think we should support Rails 2.3 to Rails |
|
@dtognazzini I agree that we should support Rails 2.3 to Rails for now. but "rails support question" I mean is about rails engine routing support, rails engine is introduced since rails 3.0, we can not provide the engine routing support in application_router.rb for rails 2.3. However, the dummy forum engine use isolate_namespace method which is introduced since rails 3.1(isolate_namespace is used in our engine codebase pretty common) https://github.com/appfolio/ae_page_objects/blob/fixRouter2/test/test_apps/shared/engines/forum/lib/forum/engine.rb#L3 . So the question is: do we need support rails 3.0? if "yes", we may figure out an alternative way to do isolate_namespace in the dummy forum engine? |
|
Since this pull is a bug fix, we should aim to support whatever we supported prior to this fix. That is, this fix shouldn't remove support for anything. That said, if the previous implementation did not support Rails 3.0 engines, then this fix doesn't need to add support. I still haven't spent any time looking at this, but generally, my comments hold for bug fixes. |
| routes.send("#{named_route}_path", *args) | ||
| rescue NoMethodError | ||
| nil | ||
| end |
There was a problem hiding this comment.
Please revert this change. NoMethodError could be raised from any number of places; handling it generically like this means you could end up handling a case you didn't mean to. The previous implementation using respond_to? was specific to what you actually need to check.
|
Please review: Generally, CodeClimate ratings should improve. |
|
I think it's time to break up application_router.rb into more files. It's incredibly difficult to read. |
| @@ -77,18 +118,80 @@ def routes | |||
| @routes ||= begin | |||
| routes_class = Class.new do | |||
| include ::Rails.application.routes.url_helpers | |||
|
|
|||
| def method_missing(name, *args, &block) | |||
| engine, named_route = name.to_s.split("__") | |||
There was a problem hiding this comment.
I don't understand why you're splitting on "__". Can you add a comment describing why? Or, better, you could extract this into a "parser" class and reuse it throughout this file.
No description provided.