Skip to content

Commit

Permalink
Reduce 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 Dec 1, 2016
1 parent b511add commit dba6064
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 44 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): Reduce warnings caused by instance variables not initialized - [@cpetschnig](https://github.com/cpetschnig).

0.18.0 (10/7/2016)
==================
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
1 change: 1 addition & 0 deletions lib/grape/middleware/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class Base
def initialize(app, **options)
@app = app
@options = default_options.merge(**options)
@app_response = nil
end

def default_options
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

0 comments on commit dba6064

Please sign in to comment.