Refactor: Simplify Setting Methods and Remove Dynamic Method Generation

CHANGELOG.md

@@ -11,6 +11,8 @@
 * [#2582](https://github.com/ruby-grape/grape/pull/2582): Fix leaky slash when normalizing - [@ericproulx](https://github.com/ericproulx).
 * [#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/api/instance.rb

@@ -232,22 +232,19 @@ def collect_route_config_per_pattern(all_routes)
         end
       end
 
+      ROOT_PREFIX_VERSIONING_KEY = %i[version version_options root_prefix].freeze
+      private_constant :ROOT_PREFIX_VERSIONING_KEY
+
       # Allows definition of endpoints that ignore the versioning configuration
       # used by the rest of your API.
       def without_root_prefix_and_versioning
-        old_version = self.class.namespace_inheritable(:version)
-        old_version_options = self.class.namespace_inheritable(:version_options)
-        old_root_prefix = self.class.namespace_inheritable(:root_prefix)
-
-        self.class.namespace_inheritable_to_nil(:version)
-        self.class.namespace_inheritable_to_nil(:version_options)
-        self.class.namespace_inheritable_to_nil(:root_prefix)
-
+        inheritable_setting = self.class.inheritable_setting
+        deleted_values = inheritable_setting.namespace_inheritable.delete(*ROOT_PREFIX_VERSIONING_KEY)
         yield
-
-        self.class.namespace_inheritable(:version, old_version)
-        self.class.namespace_inheritable(:version_options, old_version_options)
-        self.class.namespace_inheritable(:root_prefix, old_root_prefix)
+      ensure
+        ROOT_PREFIX_VERSIONING_KEY.each_with_index do |key, index|
+          inheritable_setting.namespace_inheritable[key] = deleted_values[index]
+        end
       end
     end
   end

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,64 +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]
-      def unset(type, key)
-        setting = inheritable_setting.__send__(type)
-        setting.delete key
+      def namespace_inheritable(key, value = nil)
+        get_or_set(inheritable_setting.namespace_inheritable, key, value)
       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_stackable(key, value = nil)
+        get_or_set(inheritable_setting.namespace_stackable, key, value)
       end
 
-      # defines the following methods:
-      # - namespace_inheritable
-      # - namespace_stackable
-
-      %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
 
-      def unset_namespace_stackable(*keys)
-        keys.each do |key|
-          unset :namespace_stackable, key
-        end
+      def route_setting(key, value = nil)
+        get_or_set(inheritable_setting.route, key, value)
       end
 
-      # defines the following methods:
-      # - global_setting
-      # - route_setting
-      # - namespace_setting
-
-      %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
-      end
-
-      # @param key [Symbol]
-      def namespace_inheritable_to_nil(key)
-        inheritable_setting.namespace_inheritable[key] = nil
+      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) }
@@ -107,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

lib/grape/dsl/validations.rb

@@ -22,7 +22,7 @@ module Validations
       #      # whatever
       #    end
       def reset_validations!
-        unset_namespace_stackable :declared_params, :params, :validations
+        inheritable_setting.namespace_stackable.delete(:declared_params, :params, :validations)
       end
 
       # Opens a root-level ParamsScope, defining parameter coercions and

lib/grape/util/base_inheritable.rb

@@ -14,8 +14,11 @@ def initialize(inherited_values = nil)
         @new_values = {}
       end
 
-      def delete(key)
-        new_values.delete key
+      def delete(*keys)
+        keys.map do |key|
+          # since delete returns the deleted value, seems natural to `map` the result
+          new_values.delete key
+        end
       end
 
       def initialize_copy(other)

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,57 +15,29 @@ def reset_validations!; end
     end
   end
 
-  describe '#unset' do
-    it 'deletes a key from settings' do
-      subject.namespace_setting :dummy, 1
-      expect(subject.inheritable_setting.namespace.keys).to include(:dummy)
-
-      subject.unset :namespace, :dummy
-      expect(subject.inheritable_setting.namespace.keys).not_to include(:dummy)
-    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
@@ -88,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)
@@ -108,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)
@@ -126,13 +88,6 @@ def reset_validations!; end
     end
   end
 
-  describe '#unset_namespace_stackable' do
-    it 'delegates to unset' do
-      expect(subject).to receive(:unset).with(:namespace_stackable, :dummy)
-      subject.unset_namespace_stackable(:dummy)
-    end
-  end
-
   describe 'complex scenario' do
     it 'plays well' do
       obj1 = dummy_class.new

spec/grape/dsl/validations_spec.rb

@@ -5,17 +5,23 @@
 
   let(:dummy_class) do
     Class.new do
+      extend Grape::DSL::Settings
       extend Grape::DSL::Validations
-      def self.unset_namespace_stackable(*_keys); end
     end
   end
 
   describe '.reset_validations' do
     subject { dummy_class.reset_validations! }
 
+    before do
+      %i[declared_params params validations other].each do |key|
+        dummy_class.inheritable_setting.namespace_stackable[key] = key
+      end
+    end
+
     it 'calls unset_namespace_stackable properly' do
-      expect(dummy_class).to receive(:unset_namespace_stackable).with(:declared_params, :params, :validations)
       subject
+      expect(dummy_class.inheritable_setting.namespace_stackable.to_hash).to eq(other: [:other])
     end
   end