make mouting work #28

Merged
merged 2 commits into from Jan 23, 2013

Projects

None yet

2 participants

@grosser
Member
grosser commented Nov 8, 2012

No description provided.

@jamesarosen jamesarosen commented on the diff Jan 21, 2013
config/routes.rb
-# In 3.0.1, this is not yet available.
-
-# TODO replace this with the commented-out version below
-Rails.application.routes.draw do
- resources :features, :controller => 'arturo/features'
- put 'features', :to => 'arturo/features#update_all', :as => 'features'
+if Rails::VERSION::MAJOR == 2 or (Rails::VERSION::MAJOR == 3 and Rails::VERSION::MINOR == 0)
+ Rails.application.routes.draw do
+ resources :features, :controller => 'arturo/features'
+ put 'features', :to => 'arturo/features#update_all', :as => 'features'
+ end
+else
+ Arturo::Engine.routes.draw do
+ resources :features, :controller => 'arturo/features'
+ put 'features', :to => 'arturo/features#update_all', :as => 'features'
+ end
@jamesarosen
jamesarosen Jan 21, 2013 Contributor

How about using feature detection instead of checking the Rails version?

if Arturo::Engine.respond_to?(:routes) && Arturo::Engine.routes.respond_to?(:draw)
@grosser
grosser Jan 21, 2013 Member

Meh, it's a hack for 1 specific version, I'd rather explicitly show why it is there.

@jamesarosen
jamesarosen Jan 22, 2013 Contributor

The if is there because the Rails API changed. Let's reflect the API changes rather than trying to point out Rails's problems with semantic versioning.

@jamesarosen jamesarosen and 1 other commented on an outdated diff Jan 21, 2013
app/controllers/arturo/features_controller.rb
@@ -16,6 +16,15 @@ class FeaturesController < ApplicationController
before_filter :require_permission
before_filter :load_feature, :only => [ :show, :edit, :update, :destroy ]
+ def arturo_engine
+ if defined?(super)
+ super
+ else
+ self
+ end
+ end
+ helper_method :arturo_engine
@jamesarosen
jamesarosen Jan 21, 2013 Contributor

How about

unless instance_methods.include?(:arturo_engine)
  def arturo_engine
    self
  end
end
@grosser
grosser Jan 21, 2013 Member

How about:

def arturo_engine
  self
end unless defined? arturo_engine
@jamesarosen
jamesarosen Jan 22, 2013 Contributor

Looks great.

@grosser
Member
grosser commented Jan 23, 2013

all applied

@grosser
Member
grosser commented Jan 23, 2013

I think this needs to be tested with an existing project like monitor before merging, not sure if it actually worked perfectly or if there are still issues left

@jamesarosen
Contributor

Tests pass on monitor using ee57dd7

@jamesarosen jamesarosen merged commit deeb4f9 into zendesk:master Jan 23, 2013
@jamesarosen jamesarosen was assigned May 1, 2013
@jamesarosen jamesarosen was unassigned by zd-jparas Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment