Refactored to rely less on registering plugins.

core/lib/refinery/plugin.rb

@@ -5,10 +5,10 @@ class Plugin
                   :always_allow_access, :menu_match, :hide_from_menu,
                   :pathname
 
-    def self.register(&block)
-      yield(plugin = self.new)
+    def self.register(&_block)
+      yield(plugin = new)
 
-      raise "A plugin MUST have a name!: #{plugin.inspect}" if plugin.name.blank?
+      raise ArgumentError, "A plugin MUST have a name!: #{plugin.inspect}" if plugin.name.blank?
 
       # Set defaults.
       plugin.menu_match ||= %r{refinery/#{plugin.name}(/.+?)?$}
@@ -30,11 +30,11 @@ def description
     end
 
     # Stores information that can be used to retrieve the latest activities of this plugin
-    def activity=(activities)
+    def activity=(_)
       Refinery.deprecate('Refinery::Plugin#activity=', when: '3.1')
     end
 
-    def dashboard=(dashboard)
+    def dashboard=(_)
       Refinery.deprecate('Refinery::Plugin#dashboard=', when: '3.1')
     end
 
@@ -57,17 +57,14 @@ def url
       @url ||= build_url
 
       if @url.is_a?(Hash)
-        {:only_path => true}.merge(@url)
+        { only_path: true }.merge(@url)
       elsif @url.respond_to?(:call)
         @url.call
       else
         @url
       end
     end
 
-    # Make this protected, so that only Plugin.register can use it.
-    protected
-
     def initialize
       # provide a default pathname to where this plugin is using its lib directory.
       depth = RUBY_VERSION >= "1.9.2" ? 4 : 3
@@ -77,13 +74,11 @@ def initialize
     private
 
     def build_url
-      if controller.present?
-        { :controller => "refinery/admin/#{controller}" }
-      elsif directory.present?
-        { :controller => "refinery/admin/#{directory.split('/').pop}" }
-      else
-        { :controller => "refinery/admin/#{name}" }
-      end
+      action = controller.presence ||
+               directory.to_s.split('/').pop.presence ||
+               name
+
+      { controller: "refinery/admin/#{action}" }
     end
   end
 end

core/spec/lib/refinery/plugin_spec.rb

@@ -30,7 +30,7 @@ module Refinery
 
     describe '.register' do
       it 'must have a name' do
-        expect { Plugin.register {} }.to raise_error
+        expect { Plugin.register {} }.to raise_error(ArgumentError)
       end
     end
 

core/spec/lib/refinery/plugins_spec.rb

@@ -21,98 +21,81 @@ class Engine < ::Rails::Engine
     end
   end
 
-  module MyOrderedPlugin
-    class Engine < ::Rails::Engine
-      isolate_namespace ::Refinery
-      ::Refinery::Plugin.register do |plugin|
-        plugin.name = "my_ordered_plugin"
-      end
-    end
-  end
-
-  module MyOtherOrderedPlugin
-    class Engine < ::Rails::Engine
-      isolate_namespace ::Refinery
-      ::Refinery::Plugin.register do |plugin|
-        plugin.name = "my_other_ordered_plugin"
-      end
-    end
-  end
-
-  module MyUnorderedPlugin
-    class Engine < ::Rails::Engine
-      isolate_namespace ::Refinery
-      ::Refinery::Plugin.register do |plugin|
-        plugin.name = "my_unordered_plugin"
-      end
-    end
-  end
-
   ::I18n.backend.store_translations :en, :refinery => {
     :plugins => {
       :my_plugin => {
         :title => "my plugin"
       },
       :my_other_plugin => {
         :title => "my other plugin"
-      },
-      :my_ordered_plugin => {
-        :title => "my ordered plugin"
-      },
-      :my_other_ordered_plugin => {
-        :title => "my other ordered plugin"
-      },
-      :my_unodered_plugin => {
-        :title => "my unordered plugin"
       }
     }
   }
 
   RSpec.describe Plugins do
-    subject { Refinery::Plugins }
+    def mock_plugin(name)
+      ::Refinery::Plugin.new.tap do |plugin|
+        plugin.name = name
+      end
+    end
 
     describe '#registered' do
       it 'identifies as Refinery::Plugins' do
-        expect(subject.registered.class).to eq(subject)
+        expect(described_class.registered.class).to eq(described_class)
       end
     end
 
     describe '#always_allowed' do
       it 'should identify as Refinery::Plugins' do
-        expect(subject.always_allowed.class).to eq(subject)
+        expect(described_class.always_allowed.class).to eq(described_class)
       end
 
       it 'only contains items that are always allowed' do
-        expect(subject.always_allowed.any?).to be_truthy
-        expect(subject.always_allowed.all?(&:always_allow_access)).to be_truthy
+        expect(described_class.always_allowed.any?).to be_truthy
+        expect(described_class.always_allowed.all?(&:always_allow_access)).to be_truthy
       end
     end
 
     describe '#in_menu' do
       it 'identifies as Refinery::Plugins' do
-        expect(subject.registered.in_menu.class).to eq(subject)
+        expect(described_class.registered.in_menu.class).to eq(described_class)
       end
 
       it 'only contains items that are in the menu' do
-        expect(subject.registered.in_menu.any?).to be_truthy
-        expect(subject.registered.in_menu.all? { |p| !p.hide_from_menu }).to be_truthy
+        expect(described_class.registered.in_menu.any?).to be_truthy
+        expect(described_class.registered.in_menu.all? { |p| !p.hide_from_menu }).to be_truthy
       end
 
       it "orders by plugin_priority config" do
-        Core.config.plugin_priority = %w(my_ordered_plugin my_plugin my_other_ordered_plugin)
-        expect(subject.registered.in_menu.names.take(2)).to eq %w(my_ordered_plugin my_other_ordered_plugin)
+        allow(described_class).to receive(:registered).and_return(
+          described_class.new([
+            mock_plugin("one"),
+            mock_plugin("two"),
+            mock_plugin("three"),
+            mock_plugin("four"),
+            mock_plugin("five")
+          ])
+        )
+
+        expect {
+          Core.config.plugin_priority = %w(three five two four)
+        }.to change { described_class.registered.in_menu.names }.from(
+          %w(one two three four five)
+        ).to(
+          %w(three five two four one)
+        )
       end
     end
 
     describe ".find_by_name" do
       it "finds plugin by given name" do
-        expect(subject.registered.find_by_name("my_plugin").name).to eq("my_plugin")
+        expect(described_class.registered.find_by_name("my_plugin").name).to eq("my_plugin")
       end
     end
 
     describe ".find_by_title" do
       it "finds plugin by given title" do
-        expect(subject.registered.find_by_title("my plugin").title).to eq("my plugin")
+        expect(described_class.registered.find_by_title("my plugin").title).to eq("my plugin")
       end
     end
 

pages/lib/refinery/pages/tab.rb

@@ -21,8 +21,8 @@ def self.register(&block)
 
         yield tab
 
-        raise "A tab MUST have a name!: #{tab.inspect}" if tab.name.blank?
-        raise "A tab MUST have a partial!: #{tab.inspect}" if tab.partial.blank?
+        raise ArgumentError, "A tab MUST have a name!: #{tab.inspect}" if tab.name.blank?
+        raise ArgumentError, "A tab MUST have a partial!: #{tab.inspect}" if tab.partial.blank?
 
         tab.templates = %w[all] if tab.templates.blank?
         tab.templates = Array(tab.templates)

pages/spec/lib/refinery/pages/tab_spec.rb

@@ -54,15 +54,15 @@ module Pages
             Refinery::Pages::Tab.register do |tab|
               tab.partial = "rspec"
             end
-          }.to raise_error
+          }.to raise_error(ArgumentError)
         end
 
         it "requires partial to be set" do
           expect {
             Refinery::Pages::Tab.register do |tab|
               tab.name = "rspec"
             end
-          }.to raise_error
+          }.to raise_error(ArgumentError)
         end
 
         it "sets #templates if it's not set" do