Skip to content

Commit

Permalink
Remove double splat operators when not needed including specs
Browse files Browse the repository at this point in the history
Grape::Validations::Validators::Base last arguments is now just a simple param
Grape::Exceptions::Validation and Grape::Exceptions::ValidationErrors does not end with **args.
Grape::Request has a named param build_params_with: nil instead of **options
Remove @namespace_description in routing.rb
  • Loading branch information
ericproulx committed Nov 2, 2024
1 parent 0477baf commit 7862598
Show file tree
Hide file tree
Showing 37 changed files with 133 additions and 161 deletions.
19 changes: 11 additions & 8 deletions benchmark/compile_many_routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,23 @@
$LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib'))
require 'grape'
require 'benchmark/ips'
require 'memory_profiler'

class API < Grape::API
prefix :api
version 'v1', using: :path

2000.times do |index|
get "/test#{index}/" do
'hello'
namespace :test do
2000.times do |index|
namespace index do
get do
'hello'
end
end
end
end
end

Benchmark.ips do |ips|
ips.report('Compiling 2000 routes') do
API.compile!
end
end
MemoryProfiler.report(allow_files: 'grape') do
API.compile!
end.pretty_print(to_file: 'without_double_splat.txt')
2 changes: 1 addition & 1 deletion lib/grape/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def const_missing(*args)
# For instance, a description could be done using: `desc configuration[:description]` if it may vary
# depending on where the endpoint is mounted. Use with care, if you find yourself using configuration
# too much, you may actually want to provide a new API rather than remount it.
def mount_instance(**opts)
def mount_instance(opts = {})
instance = Class.new(@base_parent)
instance.configuration = Grape::Util::EndpointConfiguration.new(opts[:configuration] || {})
instance.base = self
Expand Down
16 changes: 7 additions & 9 deletions lib/grape/api/instance.rb
Original file line number Diff line number Diff line change
Expand Up @@ -202,18 +202,16 @@ def add_head_not_allowed_methods_and_options_methods
without_root_prefix do
without_versioning do
versioned_route_configs.each do |config|
next if config[:options][:matching_wildchar]
next if config.dig(:options, :matching_wildchar)

allowed_methods = config[:methods].dup

allowed_methods |= [Rack::HEAD] if !self.class.namespace_inheritable(:do_not_route_head) && allowed_methods.include?(Rack::GET)

allow_header = (self.class.namespace_inheritable(:do_not_route_options) ? allowed_methods : [Rack::OPTIONS] | allowed_methods)

config[:endpoint].options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Rack::OPTIONS)

attributes = config.merge(allowed_methods: allowed_methods, allow_header: allow_header)
generate_not_allowed_method(config[:pattern], **attributes)
config[:allowed_methods] = allowed_methods
config[:allow_header] = allow_header
generate_not_allowed_method(config[:pattern], config)
end
end
end
Expand All @@ -240,15 +238,15 @@ def collect_route_config_per_pattern

# Generate a route that returns an HTTP 405 response for a user defined
# path on methods not specified
def generate_not_allowed_method(pattern, allowed_methods: [], **attributes)
def generate_not_allowed_method(pattern, options)
supported_methods =
if self.class.namespace_inheritable(:do_not_route_options)
Grape::Http::Headers::SUPPORTED_METHODS
else
Grape::Http::Headers::SUPPORTED_METHODS_WITHOUT_OPTIONS
end
not_allowed_methods = supported_methods - allowed_methods
@router.associate_routes(pattern, not_allowed_methods: not_allowed_methods, **attributes)
options[:not_allowed_methods] = supported_methods - options[:allowed_methods]
@router.associate_routes(pattern, options)
end

# Allows definition of endpoints that ignore the versioning configuration
Expand Down
15 changes: 7 additions & 8 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ def self.post_filter_methods(type)
# has completed
module PostBeforeFilter
def declared(passed_params, options = {}, declared_params = nil, params_nested_path = [])
options = options.reverse_merge(include_missing: true, include_parent_namespaces: true, evaluate_given: false)
declared_params ||= optioned_declared_params(**options)
options.reverse_merge!(include_missing: true, include_parent_namespaces: true, evaluate_given: false)
declared_params ||= optioned_declared_params(options[:include_parent_namespaces])

res = if passed_params.is_a?(Array)
declared_array(passed_params, options, declared_params, params_nested_path)
Expand Down Expand Up @@ -120,8 +120,8 @@ def optioned_param_key(declared_param, options)
options[:stringify] ? declared_param.to_s : declared_param.to_sym
end

def optioned_declared_params(**options)
declared_params = if options[:include_parent_namespaces]
def optioned_declared_params(include_parent_namespaces)
declared_params = if include_parent_namespaces
# Declared params including parent namespaces
route_setting(:declared_params)
else
Expand Down Expand Up @@ -199,10 +199,9 @@ def rack_response(message, status = 200, headers = { Rack::CONTENT_TYPE => conte
# Redirect to a new url.
#
# @param url [String] The url to be redirect.
# @param options [Hash] The options used when redirect.
# :permanent, default false.
# :body, default a short message including the URL.
def redirect(url, permanent: false, body: nil, **_options)
# @param permanent [Boolean] default false.
# @param body default a short message including the URL.
def redirect(url, permanent: false, body: nil)
body_message = body
if permanent
status 301
Expand Down
4 changes: 2 additions & 2 deletions lib/grape/dsl/parameters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def requires(*attrs, &block)
require_required_and_optional_fields(attrs.first, opts)
else
validate_attributes(attrs, opts, &block)
block ? new_scope(orig_attrs, &block) : push_declared_params(attrs, **opts.slice(:as))
block ? new_scope(orig_attrs, &block) : push_declared_params(attrs, opts.slice(:as))
end
end

Expand All @@ -162,7 +162,7 @@ def optional(*attrs, &block)
else
validate_attributes(attrs, opts, &block)

block ? new_scope(orig_attrs, true, &block) : push_declared_params(attrs, **opts.slice(:as))
block ? new_scope(orig_attrs, true, &block) : push_declared_params(attrs, opts.slice(:as))
end
end

Expand Down
17 changes: 5 additions & 12 deletions lib/grape/dsl/routing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,19 +175,12 @@ def route(methods, paths = ['/'], route_options = {}, &block)
# end
# end
def namespace(space = nil, options = {}, &block)
@namespace_description = nil unless instance_variable_defined?(:@namespace_description) && @namespace_description

if space || block
within_namespace do
previous_namespace_description = @namespace_description
@namespace_description = (@namespace_description || {}).deep_merge(namespace_setting(:description) || {})
nest(block) do
namespace_stackable(:namespace, Namespace.new(space, **options)) if space
end
@namespace_description = previous_namespace_description
return Namespace.joined_space_path(namespace_stackable(:namespace)) unless space || block

within_namespace do
nest(block) do
namespace_stackable(:namespace, Namespace.new(space, options)) if space
end
else
Namespace.joined_space_path(namespace_stackable(:namespace))
end
end

Expand Down
13 changes: 5 additions & 8 deletions lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def mount_in(router)
methods = [route.request_method]
methods << Rack::HEAD if !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET
methods.each do |method|
route = Grape::Router::Route.new(method, route.origin, **route.attributes.to_h) unless route.request_method == method
route = Grape::Router::Route.new(method, route.origin, route.attributes.to_h) unless route.request_method == method
router.append(route.apply(self))
end
end
Expand All @@ -164,8 +164,9 @@ def to_routes
route_options = prepare_default_route_attributes
map_routes do |method, path|
path = prepare_path(path)
params = merge_route_options(**route_options.merge(suffix: path.suffix))
route = Router::Route.new(method, path.path, **params)
route_options[:suffix] = path.suffix
params = options[:route_options].merge(route_options)
route = Router::Route.new(method, path.path, params)
route.apply(self)
end.flatten
end
Expand Down Expand Up @@ -196,10 +197,6 @@ def prepare_version
version.length == 1 ? version.first : version
end

def merge_route_options(**default)
options[:route_options].clone.merge!(**default)
end

def map_routes
options[:method].map { |method| options[:path].map { |path| yield method, path } }
end
Expand Down Expand Up @@ -411,7 +408,7 @@ def validations
return enum_for(:validations) unless block_given?

route_setting(:saved_validations)&.each do |saved_validation|
yield Grape::Validations::ValidatorFactory.create_validator(**saved_validation)
yield Grape::Validations::ValidatorFactory.create_validator(saved_validation)
end
end

Expand Down
48 changes: 18 additions & 30 deletions lib/grape/exceptions/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class Base < StandardError

attr_reader :status, :headers

def initialize(status: nil, message: nil, headers: nil, **_options)
def initialize(status: nil, message: nil, headers: nil)
super(message)

@status = status
Expand All @@ -26,57 +26,45 @@ def [](index)
# if BASE_ATTRIBUTES_KEY.key respond to a string message, then short_message is returned
# if BASE_ATTRIBUTES_KEY.key respond to a Hash, means it may have problem , summary and resolution
def compose_message(key, **attributes)
short_message = translate_message(key, **attributes)
if short_message.is_a? Hash
@problem = problem(key, **attributes)
@summary = summary(key, **attributes)
@resolution = resolution(key, **attributes)
[['Problem', @problem], ['Summary', @summary], ['Resolution', @resolution]].each_with_object(+'') do |detail_array, message|
message << "\n#{detail_array[0]}:\n #{detail_array[1]}" unless detail_array[1].blank?
message
end
else
short_message
end
end
short_message = translate_message(key, attributes)
return short_message unless short_message.is_a?(Hash)

def problem(key, **attributes)
translate_message(:"#{key}.problem", **attributes)
each_steps(key, attributes).with_object(+'') do |detail_array, message|
message << "\n#{detail_array[0]}:\n #{detail_array[1]}" unless detail_array[1].blank?
end
end

def summary(key, **attributes)
translate_message(:"#{key}.summary", **attributes)
end
def each_steps(key, attributes)
return enum_for(:each_steps, key, attributes) unless block_given?

def resolution(key, **attributes)
translate_message(:"#{key}.resolution", **attributes)
yield 'Problem', translate_message(:"#{key}.problem", attributes)
yield 'Summary', translate_message(:"#{key}.summary", attributes)
yield 'Resolution', translate_message(:"#{key}.resolution", attributes)
end

def translate_attributes(keys, **options)
def translate_attributes(keys, options = {})
keys.map do |key|
translate("#{BASE_ATTRIBUTES_KEY}.#{key}", default: key, **options)
translate("#{BASE_ATTRIBUTES_KEY}.#{key}", options.merge(default: key.to_s))
end.join(', ')
end

def translate_message(key, **options)
def translate_message(key, options = {})
case key
when Symbol
translate("#{BASE_MESSAGES_KEY}.#{key}", default: '', **options)
translate("#{BASE_MESSAGES_KEY}.#{key}", options.merge(default: ''))
when Proc
key.call
else
key
end
end

def translate(key, **options)
options = options.dup
options[:default] &&= options[:default].to_s
def translate(key, options)
message = ::I18n.translate(key, **options)
message.presence || fallback_message(key, **options)
message.presence || fallback_message(key, options)
end

def fallback_message(key, **options)
def fallback_message(key, options)
if ::I18n.enforce_available_locales && ::I18n.available_locales.exclude?(FALLBACK_LOCALE)
key
else
Expand Down
9 changes: 5 additions & 4 deletions lib/grape/exceptions/validation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@

module Grape
module Exceptions
class Validation < Grape::Exceptions::Base
class Validation < Base
attr_accessor :params, :message_key

def initialize(params:, message: nil, **args)
def initialize(params:, message: nil, status: nil, headers: nil)
@params = params
if message
@message_key = message if message.is_a?(Symbol)
args[:message] = translate_message(message)
message = translate_message(message)
end
super(**args)

super(status: status, message: message, headers: headers)
end

# Remove all the unnecessary stuff from Grape::Exceptions::Base like status
Expand Down
4 changes: 2 additions & 2 deletions lib/grape/exceptions/validation_errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@

module Grape
module Exceptions
class ValidationErrors < Grape::Exceptions::Base
class ValidationErrors < Base
ERRORS_FORMAT_KEY = 'grape.errors.format'
DEFAULT_ERRORS_FORMAT = '%<attributes>s %<message>s'

include Enumerable

attr_reader :errors

def initialize(errors: [], headers: {}, **_options)
def initialize(errors: [], headers: {})
@errors = errors.group_by(&:params)
super(message: full_messages.join(', '), status: 400, headers: headers)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/namespace.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class Namespace
# @option options :requirements [Hash] param-regex pairs, all of which must
# be met by a request's params for all endpoints in this namespace, or
# validation will fail and return a 422.
def initialize(space, **options)
def initialize(space, options)
@space = space.to_s
@options = options
end
Expand Down
4 changes: 2 additions & 2 deletions lib/grape/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ class Request < Rack::Request

alias rack_params params

def initialize(env, **options)
extend options[:build_params_with] || Grape.config.param_builder
def initialize(env, build_params_with: nil)
extend build_params_with || Grape.config.param_builder
super(env)
end

Expand Down
4 changes: 2 additions & 2 deletions lib/grape/router.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ def append(route)
map[route.request_method] << route
end

def associate_routes(pattern, **options)
Grape::Router::GreedyRoute.new(pattern: pattern, **options).then do |greedy_route|
def associate_routes(pattern, options)
Grape::Router::GreedyRoute.new(pattern, options).then do |greedy_route|
@neutral_regexes << greedy_route.to_regexp(@neutral_map.length)
@neutral_map << greedy_route
end
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/router/base_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class BaseRoute

attr_reader :index, :pattern, :options

def initialize(**options)
def initialize(options)
@options = ActiveSupport::OrderedOptions.new.update(options)
end

Expand Down
4 changes: 2 additions & 2 deletions lib/grape/router/greedy_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
module Grape
class Router
class GreedyRoute < BaseRoute
def initialize(pattern:, **options)
def initialize(pattern, options)
@pattern = pattern
super(**options)
super(options)
end

# Grape::Router:Route defines params as a function
Expand Down
Loading

0 comments on commit 7862598

Please sign in to comment.