Merge pull request #2599 from ruby-grape/refactor_setting_get_or_set

Refactor: Simplify Setting Methods and Remove Dynamic Method Generation

Author: dblock

CHANGELOG.md

@@ -12,6 +12,7 @@
 * [#2583](https://github.com/ruby-grape/grape/pull/2583): Optimize api parameter documentation and memory usage - [@ericproulx](https://github.com/ericproulx).
 * [#2589](https://github.com/ruby-grape/grape/pull/2589): Replace `send` by `__send__` in codebase - [@ericproulx](https://github.com/ericproulx).
 * [#2598](https://github.com/ruby-grape/grape/pull/2598): Refactor settings DSL to use explicit methods instead of dynamic generation - [@ericproulx](https://github.com/ericproulx).
+* [#2599](https://github.com/ruby-grape/grape/pull/2599): Simplify settings DSL get_or_set method and optimize logger implementation - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 #### Fixes

lib/grape/dsl/logger.rb

@@ -7,10 +7,11 @@ module Logger
       # method will create a new one, logging to stdout.
       # @param logger [Object] the new logger to use
       def logger(logger = nil)
+        global_settings = inheritable_setting.global
         if logger
-          global_setting(:logger, logger)
+          global_settings[:logger] = logger
         else
-          global_setting(:logger) || global_setting(:logger, ::Logger.new($stdout))
+          global_settings[:logger] || global_settings[:logger] = ::Logger.new($stdout)
         end
       end
     end

lib/grape/dsl/settings.rb

@@ -26,46 +26,32 @@ def inheritable_setting
         @inheritable_setting ||= Grape::Util::InheritableSetting.new.tap { |new_settings| new_settings.inherit_from top_level_setting }
       end
 
-      # @param type [Symbol]
-      # @param key [Symbol]
-      # @param value [Object] will be stored if the value is currently empty
-      # @return either the old value, if it wasn't nil, or the given value
-      def get_or_set(type, key, value)
-        setting = inheritable_setting.__send__(type)
-        if value.nil?
-          setting[key]
-        else
-          setting[key] = value
-        end
+      def namespace_inheritable(key, value = nil)
+        get_or_set(inheritable_setting.namespace_inheritable, key, value)
       end
 
-      # defines the following methods:
-      # - namespace_inheritable
-      # - namespace_stackable
+      def namespace_stackable(key, value = nil)
+        get_or_set(inheritable_setting.namespace_stackable, key, value)
+      end
 
-      %i[namespace_inheritable namespace_stackable].each do |method_name|
-        define_method method_name do |key, value = nil|
-          get_or_set method_name, key, value
-        end
+      def global_setting(key, value = nil)
+        get_or_set(inheritable_setting.global, key, value)
       end
 
-      # defines the following methods:
-      # - global_setting
-      # - route_setting
-      # - namespace_setting
+      def route_setting(key, value = nil)
+        get_or_set(inheritable_setting.route, key, value)
+      end
 
-      %i[global route namespace].each do |method_name|
-        define_method :"#{method_name}_setting" do |key, value = nil|
-          get_or_set method_name, key, value
-        end
+      def namespace_setting(key, value = nil)
+        get_or_set(inheritable_setting.namespace, key, value)
       end
 
       def namespace_reverse_stackable(key, value = nil)
-        get_or_set :namespace_reverse_stackable, key, value
+        get_or_set(inheritable_setting.namespace_reverse_stackable, key, value)
       end
 
       def namespace_stackable_with_hash(key)
-        settings = get_or_set :namespace_stackable, key, nil
+        settings = namespace_stackable(key)
         return if settings.blank?
 
         settings.each_with_object({}) { |value, result| result.deep_merge!(value) }
@@ -89,6 +75,12 @@ def within_namespace
 
         result
       end
+
+      def get_or_set(setting, key, value)
+        return setting[key] if value.nil?
+
+        setting[key] = value
+      end
     end
   end
 end

spec/grape/dsl/logger_spec.rb

@@ -4,17 +4,7 @@
   let(:dummy_logger) do
     Class.new do
       extend Grape::DSL::Logger
-      def self.global_setting(key, value = nil)
-        if value
-          global_setting_hash[key] = value
-        else
-          global_setting_hash[key]
-        end
-      end
-
-      def self.global_setting_hash
-        @global_setting_hash ||= {}
-      end
+      extend Grape::DSL::Settings
     end
   end
 

spec/grape/dsl/settings_spec.rb

@@ -15,47 +15,29 @@ def reset_validations!; end
     end
   end
 
-  describe '#get_or_set' do
-    it 'sets a values' do
-      subject.get_or_set :namespace, :dummy, 1
-      expect(subject.namespace_setting(:dummy)).to eq 1
-    end
-
-    it 'returns a value when nil is new value is provided' do
-      subject.get_or_set :namespace, :dummy, 1
-      expect(subject.get_or_set(:namespace, :dummy, nil)).to eq 1
-    end
-  end
-
   describe '#global_setting' do
-    it 'delegates to get_or_set' do
-      expect(subject).to receive(:get_or_set).with(:global, :dummy, 1)
-      subject.global_setting(:dummy, 1)
+    it 'sets a value globally' do
+      subject.global_setting :some_thing, :foo_bar
+      expect(subject.global_setting(:some_thing)).to eq :foo_bar
+      subject.with_namespace do
+        subject.global_setting :some_thing, :foo_bar_baz
+        expect(subject.global_setting(:some_thing)).to eq :foo_bar_baz
+      end
+      expect(subject.global_setting(:some_thing)).to eq(:foo_bar_baz)
     end
   end
 
   describe '#route_setting' do
-    it 'delegates to get_or_set' do
-      expect(subject).to receive(:get_or_set).with(:route, :dummy, 1)
-      subject.route_setting(:dummy, 1)
-    end
-
-    it 'sets a value until the next route' do
-      subject.route_setting :some_thing, :foo_bar
-      expect(subject.route_setting(:some_thing)).to eq :foo_bar
-
-      subject.inheritable_setting.route_end
-
+    it 'sets a value until the end of a namespace' do
+      subject.with_namespace do
+        subject.route_setting :some_thing, :foo_bar
+        expect(subject.route_setting(:some_thing)).to eq :foo_bar
+      end
       expect(subject.route_setting(:some_thing)).to be_nil
     end
   end
 
   describe '#namespace_setting' do
-    it 'delegates to get_or_set' do
-      expect(subject).to receive(:get_or_set).with(:namespace, :dummy, 1)
-      subject.namespace_setting(:dummy, 1)
-    end
-
     it 'sets a value until the end of a namespace' do
       subject.with_namespace do
         subject.namespace_setting :some_thing, :foo_bar
@@ -78,11 +60,6 @@ def reset_validations!; end
   end
 
   describe '#namespace_inheritable' do
-    it 'delegates to get_or_set' do
-      expect(subject).to receive(:get_or_set).with(:namespace_inheritable, :dummy, 1)
-      subject.namespace_inheritable(:dummy, 1)
-    end
-
     it 'inherits values from surrounding namespace' do
       subject.with_namespace do
         subject.namespace_inheritable(:some_thing, :foo_bar)
@@ -98,11 +75,6 @@ def reset_validations!; end
   end
 
   describe '#namespace_stackable' do
-    it 'delegates to get_or_set' do
-      expect(subject).to receive(:get_or_set).with(:namespace_stackable, :dummy, 1)
-      subject.namespace_stackable(:dummy, 1)
-    end
-
     it 'stacks values from surrounding namespace' do
       subject.with_namespace do
         subject.namespace_stackable(:some_thing, :foo_bar)