Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

After callback warning #1285

Merged
merged 1 commit into from
Feb 22, 2016
Merged

After callback warning #1285

merged 1 commit into from
Feb 22, 2016

Conversation

gregormelhorn
Copy link

#1265

See discussion in the issue...

before
begin
@app_response = @app.call(@env)
ensure
after_response = after
begin
error = catch(:error) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the catch(:error) do rack /grape specific? Otherwise I don't see why to use this instead of re-raising the error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, throw is not rescued, so this is not necessary. I removed the extra catch.

@rngtng
Copy link
Contributor

rngtng commented Feb 22, 2016

👍 pls squash

@gregormelhorn
Copy link
Author

Done.

before do
@warnings = warnings = []
Grape::Middleware::Base.class_eval do
define_method(:warn) { |m| warnings << m }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be

allow_any_instance_of(Grape::Middleware::Base).to receive(:warn) { |m| warnings << m }

Then you don't need an after block.

@dblock
Copy link
Member

dblock commented Feb 22, 2016

Does need a CHANGELOG entry too, please.

@gregormelhorn
Copy link
Author

Alright, done 👍 I really should have thought of allow_any_instance_of, thanks for the hint!


it 'does show a warning' do
expect { subject.call({}) }.to raise_error(StandardError)
expect(@warnings).not_to be_empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change this, but it reads more English to say to_not :)

dblock added a commit that referenced this pull request Feb 22, 2016
@dblock dblock merged commit 126f561 into ruby-grape:master Feb 22, 2016
@dblock
Copy link
Member

dblock commented Feb 22, 2016

Merged.

dblock added a commit that referenced this pull request Feb 23, 2016
railsmith pushed a commit to railsmith/grape that referenced this pull request Feb 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants