-
-
Notifications
You must be signed in to change notification settings - Fork 282
Add Rails/UnprocessableContentStatus
cop
#1520
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
base: master
Are you sure you want to change the base?
Conversation
d881eba
to
a4824e5
Compare
|
||
private | ||
|
||
def rack_3_1_or_newer? |
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 believe you can use requires_gem
for this: https://docs.rubocop.org/rubocop/development.html#requiring-a-gem
You may have to also do let(:gem_versions) { { 'rack' => '3.1.0' } }
in tests to have them actually run, not 100% sure about that though
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.
TIL I learned about requires_gem
! Thanks for letting me know. It's much nicer.
# render json: { error: "Invalid data" }, status: :unprocessable_content | ||
# head :unprocessable_content | ||
# | ||
class UnprocessableContentStatus < Base |
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 it would be better to name it Rails/DeprecatedHttpStatusNames
instead of specializing it to a specific name. This way, it can be extended in the future when new deprecated status names appear.
Additionally, this cop could also check for payload_too_large
.
https://github.com/rack/rack/blob/v3.2.0/lib/rack/utils.rb#L576-L579
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.
Good suggestion! I've updated the cop to be more generally useful. I noticed the HttpStatus
cop too and took some inspiration for the updated matcher. I also considered whether it made sense to have that cop handle the deprecations, but thought the deprecation may be helpful to have separately, and to not change the isolated focus of that.
(pair (sym :status) (sym :unprocessable_entity)) | ||
PATTERN | ||
|
||
def on_sym(node) |
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.
This is only for head :unprocessable_entity
, right? redirect_to
is already handled by on_pair
, same with render
.
In that case it seems better to use on_send
with RESTRICT_ON_SEND
and simply look at the first positional argument.
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.
That is what is was for. I looked at Rails/HttpStatus
and used the same approach there, which I think probably makes the most sense.
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.
Rails actually considers both symbols to part of their public api, regardless of what rack does with them: rails/rails#53383. So the next rails releases will no longer show the warning from rack.
This could still be nice for consistency + the other status code koic mentioned.
ec4e9e4
to
cbd4725
Compare
About
In Rack
3.1.0
, the status naming for HTTP status 422 was changed from:unprocessable_entity
to:unprocessable_content
to reflect the status. Both are supported, but there is a warning message that will display if using:unprocessable_entity
.This new cop provides a way to have Rubocop register the old status name usages as offenses and auto-correct them as well.
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.If this is a new cop, consider making a corresponding update to the Rails Style Guide.