rubocop / rubocop-rails

A RuboCop extension focused on enforcing Rails best practices and coding conventions.
https://docs.rubocop.org/rubocop-rails
MIT License
802 stars 257 forks source link

Rails/HttpPositionalArguments autocorrection does the wrong thing for the "format" keyword #186

Open andreynering opened 4 years ago

andreynering commented 4 years ago

Expected behavior

This:

get :foo, id: 5, format: :json

Should be converted to:

get :foo, params: { id: 5 }, format: :json

Actual behavior

But it's actually converted to:

get :foo, params: { id: 5, format: :json }

In some situations, I've seen it been put inside the session keyword (?):

get :foo, session: { format: :json }

RuboCop version

$ bundle exec rubocop -V
0.76.0 (using Parser 2.7.0.2, running on ruby 2.5.5 x86_64-darwin18)
tejasbubane commented 4 years ago

I think format is not supported keyword. See https://api.rubyonrails.org/classes/ActionController/TestCase/Behavior.html#method-i-get It is fair for rubocop to think of it as any other parameter.

In fact, we have an explicit test-case for this: https://github.com/rubocop-hq/rubocop-rails/blob/38840ddbdcd0ac3a0b56ee4c169a10e5dca00cad/spec/rubocop/cop/rails/http_positional_arguments_spec.rb#L368-L377

schmijos commented 4 years ago

I've the same problem with a supported argument:

     def authorized_post(path, options = {})
-      post path, { headers: { 'Api-Key': api_access.key }, as: :json }.merge(options)
+      post path, params: { headers: { 'Api-Key': api_access.key }, as: :json }.merge(options)
     end

The suggested change is wrong and I don't think this is easily auto-correctable because the correction would go into this direction:

post path, params: options[:params],
           headers: { 'Api-Key': api_access.key }.merge(options[:headers] || {}),
           as: :json
tejasbubane commented 4 years ago

@schmijos This seems different from the original issue. It is mostly because of the parens and merge.