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

Rails/HttpStatus doesn't detect status key properly #5738

Closed
btoconnor opened this issue Mar 30, 2018 · 1 comment · Fixed by #5739
Closed

Rails/HttpStatus doesn't detect status key properly #5738

btoconnor opened this issue Mar 30, 2018 · 1 comment · Fixed by #5739
Labels

Comments

@btoconnor
Copy link

I recently upgraded to 0.54.0 and noticed that the new Rails/HttpStatus cop was not picking up the following issue:

render status: 201, json: {}

After spending an hour or so ensuring that everything was setup properly, I went and tried the following:

render json: {}, status: 201

and that code correctly tripped the new HttpStatus error.


Expected behavior

I expected rubocop to pick up the status parameter to render regardless of position

Actual behavior

Rubocop did not trigger a linting error. It correctly triggers a linting issue when the code is formatted as

render json: {}, status: 201

Screenshot from my editor with rubocop integrated:
image

Steps to reproduce the problem

In a controller, write

render status: 201, json: {}

with Rails/HttpStatus enabled, and EnforcedStyle set to symbolic (the default)

RuboCop version

Include the output of rubocop -V. Here's an example:

$ rubocop -V
0.54.0 (using Parser 2.5.0.5, running on ruby 2.4.2 x86_64-linux)
@pocke pocke added the bug label Mar 31, 2018
pocke added a commit to pocke/rubocop that referenced this issue Apr 1, 2018
… false negative

Fix rubocop#5738

Problem
===

Currently `Rails/HttpStatus` checks only first or second pair in hash.

```ruby
render 'index', status: 201
              # ^^^^^^^^^^^ checks
render 'index', something: foobar, status: 201
                                 # ^^^^^^^^^^^ does not check

render status: 201, json: {}
     # ^^^^^^^^^^^ does not check
render json: {}, status: 201
               # ^^^^^^^^^^^ checks
render json: {}, something: foobar, status: 201
                                  # ^^^^^^^^^^^ does not check
```

Solution
===

Make the cop to check all pairs in hash.

Note
===

Originally the cop does not check `redirect_to` with one argument (e.g.  `redirect_to action: :index, status: 301`). But it is a correct style, so the cop should add an offense for the style. This change also fixes this bug.
unkmas added a commit to unkmas/rubocop that referenced this issue Apr 1, 2018
bbatsov pushed a commit that referenced this issue Apr 2, 2018
…negative

Fix #5738

Problem
===

Currently `Rails/HttpStatus` checks only first or second pair in hash.

```ruby
render 'index', status: 201
              # ^^^^^^^^^^^ checks
render 'index', something: foobar, status: 201
                                 # ^^^^^^^^^^^ does not check

render status: 201, json: {}
     # ^^^^^^^^^^^ does not check
render json: {}, status: 201
               # ^^^^^^^^^^^ checks
render json: {}, something: foobar, status: 201
                                  # ^^^^^^^^^^^ does not check
```

Solution
===

Make the cop to check all pairs in hash.

Note
===

Originally the cop does not check `redirect_to` with one argument (e.g.  `redirect_to action: :index, status: 301`). But it is a correct style, so the cop should add an offense for the style. This change also fixes this bug.
@a14m
Copy link

a14m commented May 14, 2018

I'm still experiencing the same issue

      # POST /videos
      def create
        # ...
        render status: 201, json: video
      end

      # DELETE /videos/1
      def destroy
        # ...
        head :no_content
      end

it doesn't detect any violation (no matter whether the EnforcedStyle is set to numeric or symbolic)

$ rubocop -V
0.55.0 (using Parser 2.5.1.0, running on ruby 2.5.1 x86_64-darwin17)

Apparently it has something to do with rubocop-rspec plugin
after removing require: rubocop-rspec from .rubocop.yml it started working properly again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants