diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e7b60c49..f6e111bdb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 3c77ae398..241100a88 100644 --- a/lib/grape/api/instance.rb +++ b/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 diff --git a/lib/grape/dsl/logger.rb b/lib/grape/dsl/logger.rb index c0b38b6f1..5962f0fed 100644 --- a/lib/grape/dsl/logger.rb +++ b/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 diff --git a/lib/grape/dsl/settings.rb b/lib/grape/dsl/settings.rb index 415e4237f..81a9c98a6 100644 --- a/lib/grape/dsl/settings.rb +++ b/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 diff --git a/lib/grape/dsl/validations.rb b/lib/grape/dsl/validations.rb index 6beb8f37a..e90f5af0b 100644 --- a/lib/grape/dsl/validations.rb +++ b/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 diff --git a/lib/grape/util/base_inheritable.rb b/lib/grape/util/base_inheritable.rb index cc68ababf..c70ad20f1 100644 --- a/lib/grape/util/base_inheritable.rb +++ b/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) diff --git a/spec/grape/dsl/logger_spec.rb b/spec/grape/dsl/logger_spec.rb index 3b135c48e..ead059ac5 100644 --- a/spec/grape/dsl/logger_spec.rb +++ b/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 diff --git a/spec/grape/dsl/settings_spec.rb b/spec/grape/dsl/settings_spec.rb index 1a475a764..ecdf5a85b 100644 --- a/spec/grape/dsl/settings_spec.rb +++ b/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 diff --git a/spec/grape/dsl/validations_spec.rb b/spec/grape/dsl/validations_spec.rb index 93a55191d..301ac287c 100644 --- a/spec/grape/dsl/validations_spec.rb +++ b/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