New cop checks for braces in function calls with hash arguments.

[dblock]

It's quite common to make function calls such as where({ x: 1 }), in which case the curly braces are not required and you can write where(x: 1). The BracesInHashParameters supports both a required (default) and a none style, keeping things consistent either way.

[fancyremarker]

This cop would be very useful for me! One question: should the default be { EnforcedStyle: none } per the default style guide?

Omit the outer braces around an implicit options hash.

# bad
user.set({ name: 'John', age: 45, permissions: { read: true } })

# good
User.set(name: 'John', age: 45, permissions: { read: true })

[jonas054]

I think I'd prefer BracesAroundHashParameters. And keep the cops in alphabetical order in this file.

[jonas054]

@dblock This cop seems like a good idea. I've added some source code comments. I agree with @fancyremarker that the default should be to not have braces. Oh, and you also need to make sure that the source code passes rubocop's own inspection!

[dblock]

Updated with all the suggested changes/fixes. Enforcing the default style of BracesAroundHashParameters to none.

[jonas054]

This is actually just one hash with multiple keys, so the heading "accepts multiple hash parameters" is wrong.

[dblock]

Reviewed and updated all names.

[jonas054]

Added a few more comments. Sorry I didn't write them all at once.

I noticed you chose to stick with required/none rather than braces/no_braces. What was your motivation for that?

[dblock]

Renamed none and required to no_braces and braces, yours is better.

[dblock]

Updated, squashed. Please re-review. Happy to make more changes as you see fit.

[dblock]

Updated again, squashed. Not using a regexp anymore, thanks for insisting, this is a lot better.

[bbatsov]

@dblock :-) Code looks good now. You'll have to rebase on top of the current master, since there is a merge conflict right now.

[dblock]

Here're you go.

[bbatsov]

Thanks!

[jonas054]

This is the same as a "bad" example from the style guide, so I think it's wrong to accept it when EnforcedStyle is no_braces.

class Person < ActiveRecord::Base
  # bad
  validates(:name, { presence: true, length: { within: 1..10 } })

  # good
  validates :name, presence: true, length: { within: 1..10 }
end

[bbatsov]

Yep, I noticed yesterday that the code currently checks only methods with 1 argument, which is problematic.

[jonas054]

So we should also make the check when there are multiple arguments, and check the last argument. Do you want to look at that, @dblock?

[dblock]

I don't think this is quite right. First, the violation of parenthesis is a different beast, and not something that relates to braces. I support a cop for that of course, as it's bad :)

For the braces, you can't just check the last argument. Here's am ambiguous function where we may need to disambiguate the arguments:

def sort_of_validates(name, options, parameters = {})
 puts "NAME: #{name}"
 puts "OPTIONS: #{options}"
 puts "PARAMETERS: #{parameters}"
end

sort_of_validates :name, presence: true, length: { within: 1..10 }
# NAME: name
# OPTIONS: {:presence=>true, :length=>{:within=>1..10}}
# PARAMETERS: {}

sort_of_validates :name, { presence: true }, { length: { within: 1..10 } }
# NAME: name
# OPTIONS: {:presence=>true}
# PARAMETERS: {:length=>{:within=>1..10}}

I am not sure what to do. Thoughts?

[jonas054]

Here's how I see it. Only the last hash parameter can omit the braces. That's Ruby syntax. If you want the presence and length values to go in two different parameters, you have to put braces around presence: true. Then you can put braces around length: { within: 1..10 }, or leave them out. It won't change the meaning. It's just a style issue and that's what we're checking.

[bbatsov]

@jonas054 is totally right.

[dblock]

Ok. Thinking out loud ...

where({ x: 1 }, { y: 2 })

Should be treated as a special case and not register an offence, because it would seem weird that multiple hashes in the same expression are written with or without braces?

[dblock]

Opened #567, lets move the discussion there if there's more.