-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 response headers from lint #2414
Fix response headers from lint #2414
Conversation
Minor refactor
Replace `rack_response` to `error!`
call self.status once inside route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we do have specs that use rack_response
in rescue_from
. Someone is undoubtedly relying on it.
- Add a section to UPGRADING.md.
- Is there a way we can re-implement it after these changes and mark it deprecated instead?
Revert rack_response in inside_route with deprecation Add spec
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits, fix if you agree and hit merge
UPGRADING.md
Outdated
@@ -3,6 +3,9 @@ Upgrading Grape | |||
|
|||
### Upgrading to >= 2.1.0 | |||
|
|||
#### Changes in rescue_from | |||
`rack_response` has been deprecated and `error_response` has been removed. Use `error!` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rack_response
method ... (just to make it a sentence)
Link to the PR here and a couple missing below.
See [#2414](https://github.com/ruby-grape/grape/pull/2414) for more information.
lib/grape/dsl/inside_route.rb
Outdated
@@ -180,8 +180,9 @@ def error!(message, status = nil, additional_headers = nil) | |||
# A Rack::Response object containing the specified message, status, and headers. | |||
# | |||
def rack_response(message, status = 200, headers = { Rack::CONTENT_TYPE => content_type }) | |||
message = ERB::Util.html_escape(message) if headers[Rack::CONTENT_TYPE] == 'text/html' | |||
Rack::Response.new([message], Rack::Utils.status_code(status), headers) | |||
Grape.deprecator.warn('Use error! instead of rack_response') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this reads cleaner?
The rack_response method has been deprecated, use error! instead.
Change deprecation msg
Fix #2411
Changes:
Rack::Utils.escape_html
instead ofERB::Util.escape_html
rack_response
frominside_route
if: ActiveSupport::VERSION::MAJOR >= ?
in specs