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

Fix Grape::Middleware::Base#response #871

Merged
merged 1 commit into from
Dec 30, 2014

Conversation

Galathius
Copy link
Contributor

No description provided.

@dblock
Copy link
Member

dblock commented Dec 29, 2014

Looks legit, I guess this isn't used by anyone? Maybe we should delete it?

This needs a spec, please and a CHANGELOG entry.

@Galathius Galathius force-pushed the fix-rack-response-initialize branch 3 times, most recently from a59700d to 31385a3 Compare December 29, 2014 19:29
@ArtsemKamenka
Copy link

👍

@@ -1,7 +1,7 @@
Next Release
============

* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

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

Put this back please :)

@@ -2,6 +2,7 @@ Next Release
============

* Your contribution here.
* [#871](https://github.com/intridea/grape/pull/871): Fixed Rack::Response initialize - [@galathius](https://github.com/galathius).
Copy link
Member

Choose a reason for hiding this comment

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

I am still nitpicking, what is fixed is the Grape::Middleware::Base#response method, not Rack::Response#initialize, so this should say Fixed Grape::Middleware::Base#response - .... Also swap with the above line, future contributions will go below this one usually.

@Galathius Galathius changed the title Fix Rack::Response initialize Fix Grape::Middleware::Base#response Dec 29, 2014
@dblock
Copy link
Member

dblock commented Dec 29, 2014

Last thing, the description in the commit itself is wrong, same issue as with what was in the CHANGELOG. Thanks for hanging in there, I am a picky code-reviewer.

@@ -2,6 +2,7 @@ Next Release
============

* Your contribution here.
* [#871](https://github.com/intridea/grape/pull/871): Fixed Grape::Middleware::Base#response - [@galathius](https://github.com/galathius).
Copy link
Member

Choose a reason for hiding this comment

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

Oh and I still want you to swap the order of this line with "Your contribution here.", so that's below.

@ascrazy
Copy link

ascrazy commented Dec 30, 2014

+1

@dblock
Copy link
Member

dblock commented Dec 30, 2014

Thanks, merging.

dblock added a commit that referenced this pull request Dec 30, 2014
@dblock dblock merged commit fc5763d into ruby-grape:master Dec 30, 2014
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.

4 participants