Skip to content

Commit

Permalink
Avoid noise when code runs with Ruby warnings
Browse files Browse the repository at this point in the history
New versions of RSpec have warnings turned on by default:

    # This setting enables warnings. It's recommended, but in some cases may
    # be too noisy due to issues in dependencies.
    config.warnings = true

I believe these warnings are helpful in general to write better code. However, Grape and Grape Entity are very noisy. I get about 20 lines of warnings for one single test where Grape is involved.

I hope you agree. Many of my changes could also be solved differently for sure. I am happy to discuss and change those cases. I also changed the specs for less warnings and turned them on in the test suite.
  • Loading branch information
Christoph Petschnig committed Nov 25, 2016
1 parent b511add commit 4e234cc
Show file tree
Hide file tree
Showing 13 changed files with 82 additions and 46 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Next Release
* [#1517](https://github.com/ruby-grape/grape/pull/1517), [#1089](https://github.com/ruby-grape/grape/pull/1089): Fix: priority of ANY routes - [@namusyaka](https://github.com/namusyaka), [@wagenet](https://github.com/wagenet).
* [#1512](https://github.com/ruby-grape/grape/pull/1512): Fix: deeply nested parameters are included within `#declared(params)` - [@krbs](https://github.com/krbs).
* [#1510](https://github.com/ruby-grape/grape/pull/1510): Fix: inconsistent validation for multiple parameters - [@dgasper](https://github.com/dgasper).
* [#1526](https://github.com/ruby-grape/grape/pull/1526): Remove warnings caused by instance variables not initialized - [@cpetschnig](https://github.com/cpetschnig).

0.18.0 (10/7/2016)
==================
Expand Down
1 change: 1 addition & 0 deletions lib/grape/dsl/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def api_changed(new_api)
protected

def process_named_params
@named_params ||= nil
return unless @named_params && @named_params.any?
api.namespace_stackable(:named_params, @named_params)
end
Expand Down
6 changes: 4 additions & 2 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ def redirect(url, permanent: false, body: nil, **_options)
#
# @param status [Integer] The HTTP Status Code to return for this request.
def status(status = nil)
@status ||= nil
case status
when Symbol
raise ArgumentError, "Status code :#{status} is invalid." unless Rack::Utils::SYMBOL_TO_STATUS_CODE.keys.include?(status)
Expand Down Expand Up @@ -175,7 +176,7 @@ def body(value = nil)
@body = ''
status 204
else
@body
@body ||= nil
end
end

Expand All @@ -195,7 +196,7 @@ def file(value = nil)
warn '[DEPRECATION] Argument as FileStreamer-like object is deprecated. Use path to file instead.'
@file = Grape::ServeFile::FileResponse.new(value)
else
@file
@file ||= nil
end
end

Expand Down Expand Up @@ -253,6 +254,7 @@ def present(*args)
object
end

@body ||= nil
representation = { root => representation } if root
if key
representation = (@body || {}).merge(key => representation)
Expand Down
5 changes: 5 additions & 0 deletions lib/grape/dsl/parameters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ module Parameters
# end
# end
def use(*names)
@api ||= nil
named_params = @api.namespace_stackable_with_hash(:named_params) || {}
options = names.extract_options!
names.each do |name|
Expand Down Expand Up @@ -97,6 +98,7 @@ def use(*names)
def requires(*attrs, &block)
orig_attrs = attrs.clone

@group ||= nil
opts = attrs.extract_options!.clone
opts[:presence] = { value: true, message: opts[:message] }
opts = @group.merge(opts) if @group
Expand All @@ -117,6 +119,7 @@ def requires(*attrs, &block)
def optional(*attrs, &block)
orig_attrs = attrs.clone

@group ||= nil
opts = attrs.extract_options!.clone
type = opts[:type]
opts = @group.merge(opts) if @group
Expand Down Expand Up @@ -209,6 +212,8 @@ def map_params(params, element)
# @return hash of parameters relevant for the current scope
# @api private
def params(params)
@parent ||= nil
@element ||= nil
params = @parent.params(params) if @parent
params = map_params(params, @element) if @element
params
Expand Down
2 changes: 2 additions & 0 deletions lib/grape/dsl/routing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def version(*args, &block)
end
end

@versions ||= nil
@versions.last unless @versions.nil?
end

Expand Down Expand Up @@ -157,6 +158,7 @@ def route(methods, paths = ['/'], route_options = {}, &block)
# end
# end
def namespace(space = nil, options = {}, &block)
@namespace_description ||= nil
if space || block_given?
within_namespace do
previous_namespace_description = @namespace_description
Expand Down
2 changes: 2 additions & 0 deletions lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ def initialize(new_settings, options = {}, &block)
@options[:route_options] ||= {}

@lazy_initialize_lock = Mutex.new
@lazy_initialized = nil
@block = nil

return unless block_given?

Expand Down
5 changes: 5 additions & 0 deletions lib/grape/middleware/formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ module Middleware
class Formatter < Base
CHUNKED = 'chunked'.freeze

def initialize(*_)
super
@app_response = nil
end

def default_options
{
default_format: :txt,
Expand Down
1 change: 1 addition & 0 deletions lib/grape/validations/params_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def initialize(opts, &block)
@group = opts[:group] || {}
@dependent_on = opts[:dependent_on]
@declared_params = []
@index = nil

instance_eval(&block) if block_given?

Expand Down
2 changes: 1 addition & 1 deletion spec/grape/api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,7 @@ class PhonyMiddleware
def initialize(app, *args)
@args = args
@app = app
@block = true if block_given?
@block = block_given? ? true : nil
end

def call(env)
Expand Down
69 changes: 39 additions & 30 deletions spec/grape/middleware/formatter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,46 +4,53 @@
subject { Grape::Middleware::Formatter.new(app) }
before { allow(subject).to receive(:dup).and_return(subject) }

let(:app) { ->(_env) { [200, {}, [@body || { 'foo' => 'bar' }]] } }
let(:body) { { 'foo' => 'bar' } }
let(:app) { ->(_env) { [200, {}, [body]] } }

context 'serialization' do
let(:body) { { 'abc' => 'def' } }
it 'looks at the bodies for possibly serializable data' do
@body = { 'abc' => 'def' }
_, _, bodies = *subject.call('PATH_INFO' => '/somewhere', 'HTTP_ACCEPT' => 'application/json')
bodies.each { |b| expect(b).to eq(MultiJson.dump(@body)) }
bodies.each { |b| expect(b).to eq(MultiJson.dump(body)) }
end

it 'calls #to_json since default format is json' do
@body = ['foo']
@body.instance_eval do
def to_json
'"bar"'
context 'default format' do
let(:body) { ['foo'] }
it 'calls #to_json since default format is json' do
body.instance_eval do
def to_json
'"bar"'
end
end
end

subject.call('PATH_INFO' => '/somewhere', 'HTTP_ACCEPT' => 'application/json').to_a.last.each { |b| expect(b).to eq('"bar"') }
subject.call('PATH_INFO' => '/somewhere', 'HTTP_ACCEPT' => 'application/json').to_a.last.each { |b| expect(b).to eq('"bar"') }
end
end

it 'calls #to_json if the content type is jsonapi' do
@body = { 'foos' => [{ 'bar' => 'baz' }] }
@body.instance_eval do
def to_json
'{"foos":[{"bar":"baz"}] }'
context 'jsonapi' do
let(:body) { { 'foos' => [{ 'bar' => 'baz' }] } }
it 'calls #to_json if the content type is jsonapi' do
body.instance_eval do
def to_json
'{"foos":[{"bar":"baz"}] }'
end
end
end

subject.call('PATH_INFO' => '/somewhere', 'HTTP_ACCEPT' => 'application/vnd.api+json').to_a.last.each { |b| expect(b).to eq('{"foos":[{"bar":"baz"}] }') }
subject.call('PATH_INFO' => '/somewhere', 'HTTP_ACCEPT' => 'application/vnd.api+json').to_a.last.each { |b| expect(b).to eq('{"foos":[{"bar":"baz"}] }') }
end
end

it 'calls #to_xml if the content type is xml' do
@body = 'string'
@body.instance_eval do
def to_xml
'<bar/>'
context 'xml' do
let(:body) { 'string' }
it 'calls #to_xml if the content type is xml' do
body.instance_eval do
def to_xml
'<bar/>'
end
end
end

subject.call('PATH_INFO' => '/somewhere.xml', 'HTTP_ACCEPT' => 'application/json').to_a.last.each { |b| expect(b).to eq('<bar/>') }
subject.call('PATH_INFO' => '/somewhere.xml', 'HTTP_ACCEPT' => 'application/json').to_a.last.each { |b| expect(b).to eq('<bar/>') }
end
end
end

Expand Down Expand Up @@ -189,10 +196,12 @@ def to_xml
_, _, body = subject.call('PATH_INFO' => '/info.custom')
expect(body.body).to eq(['CUSTOM FORMAT'])
end
it 'uses default json formatter' do
@body = ['blah']
_, _, body = subject.call('PATH_INFO' => '/info.json')
expect(body.body).to eq(['["blah"]'])
context 'default' do
let(:body) { ['blah'] }
it 'uses default json formatter' do
_, _, body = subject.call('PATH_INFO' => '/info.json')
expect(body.body).to eq(['["blah"]'])
end
end
it 'uses custom json formatter' do
subject.options[:formatters][:json] = ->(_obj, _env) { 'CUSTOM JSON FORMAT' }
Expand Down Expand Up @@ -284,10 +293,10 @@ def to_xml
end

context 'send file' do
let(:app) { ->(_env) { [200, {}, @body] } }
let(:body) { Grape::ServeFile::FileResponse.new('file') }
let(:app) { ->(_env) { [200, {}, body] } }

it 'returns Grape::Uril::SendFileReponse' do
@body = Grape::ServeFile::FileResponse.new('file')
env = { 'PATH_INFO' => '/somewhere', 'HTTP_ACCEPT' => 'application/json' }
expect(subject.call(env)).to be_a(Grape::ServeFile::SendfileResponse)
end
Expand Down
25 changes: 15 additions & 10 deletions spec/grape/middleware/versioner/param_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

describe Grape::Middleware::Versioner::Param do
let(:app) { ->(env) { [200, env, env['api.version']] } }
subject { Grape::Middleware::Versioner::Param.new(app, @options || {}) }
let(:options) { {} }
subject { Grape::Middleware::Versioner::Param.new(app, options) }

it 'sets the API version based on the default param (apiver)' do
env = Rack::MockRequest.env_for('/awesome', params: { 'apiver' => 'v1' })
Expand All @@ -22,7 +23,7 @@
end

context 'with specified parameter name' do
before { @options = { version_options: { parameter: 'v' } } }
let(:options) { { version_options: { parameter: 'v' } } }
it 'sets the API version based on the custom parameter name' do
env = Rack::MockRequest.env_for('/awesome', params: { 'v' => 'v1' })
expect(subject.call(env)[1]['api.version']).to eq('v1')
Expand All @@ -34,7 +35,7 @@
end

context 'with specified versions' do
before { @options = { versions: %w(v1 v2) } }
let(:options) { { versions: %w(v1 v2) } }
it 'throws an error if a non-allowed version is specified' do
env = Rack::MockRequest.env_for('/awesome', params: { 'apiver' => 'v3' })
expect(catch(:error) { subject.call(env) }[:status]).to eq(404)
Expand All @@ -45,13 +46,17 @@
end
end

it 'returns a 200 when no version is set (matches the first version found)' do
@options = {
versions: ['v1'],
version_options: { using: :header }
}
env = Rack::MockRequest.env_for('/awesome', params: {})
expect(subject.call(env).first).to eq(200)
context 'when no version is set' do
let(:options) do
{
versions: ['v1'],
version_options: { using: :header }
}
end
it 'returns a 200 (matches the first version found)' do
env = Rack::MockRequest.env_for('/awesome', params: {})
expect(subject.call(env).first).to eq(200)
end
end

context 'when there are multiple versions without a custom param' do
Expand Down
7 changes: 4 additions & 3 deletions spec/grape/middleware/versioner/path_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

describe Grape::Middleware::Versioner::Path do
let(:app) { ->(env) { [200, env, env['api.version']] } }
subject { Grape::Middleware::Versioner::Path.new(app, @options || {}) }
let(:options) { {} }
subject { Grape::Middleware::Versioner::Path.new(app, options) }

it 'sets the API version based on the first path' do
expect(subject.call('PATH_INFO' => '/v1/awesome').last).to eq('v1')
Expand All @@ -17,7 +18,7 @@
end

context 'with a pattern' do
before { @options = { pattern: /v./i } }
let(:options) { { pattern: /v./i } }
it 'sets the version if it matches' do
expect(subject.call('PATH_INFO' => '/v1/awesome').last).to eq('v1')
end
Expand All @@ -29,7 +30,7 @@

[%w(v1 v2), [:v1, :v2], [:v1, 'v2'], ['v1', :v2]].each do |versions|
context "with specified versions as #{versions}" do
before { @options = { versions: versions } }
let(:options) { { versions: versions } }

it 'throws an error if a non-allowed version is specified' do
expect(catch(:error) { subject.call('PATH_INFO' => '/v3/awesome') }[:status]).to eq(404)
Expand Down
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,7 @@
config.include Rack::Test::Methods
config.raise_errors_for_deprecations!

config.warnings = true

config.before(:each) { Grape::Util::InheritableSetting.reset_global! }
end

0 comments on commit 4e234cc

Please sign in to comment.