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

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

Merged
merged 1 commit into from Oct 9, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### New features

* [#551](https://github.com/bbatsov/rubocop/pull/551) - New cop `BracesAroundHashParameters` checks for braces in function calls with hash parameters.

### Bugs fixed

* Fix bug concerning table and separator alignment of multi-line hash with multiple keys on the same line.
Expand Down
4 changes: 4 additions & 0 deletions config/default.yml
Expand Up @@ -184,3 +184,7 @@ TrivialAccessors:
VariableName:
# Valid values are: snake_case, camelCase
EnforcedStyle: snake_case

BracesAroundHashParameters:
# Valid values are: braces, no_braces
EnforcedStyle: no_braces
4 changes: 4 additions & 0 deletions config/enabled.yml
Expand Up @@ -482,6 +482,10 @@ WhileUntilDo:
Description: 'Checks for redundant do after while or until.'
Enabled: true

BracesAroundHashParameters:
Description: 'Enforce braces style inside hash parameters.'
Enabled: true

#################### Lint ################################
### Warnings

Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -142,6 +142,7 @@
require 'rubocop/cop/style/when_then'
require 'rubocop/cop/style/while_until_do'
require 'rubocop/cop/style/word_array'
require 'rubocop/cop/style/braces_around_hash_parameters'

require 'rubocop/cop/rails/has_and_belongs_to_many'
require 'rubocop/cop/rails/read_attribute'
Expand Down
40 changes: 40 additions & 0 deletions lib/rubocop/cop/style/braces_around_hash_parameters.rb
@@ -0,0 +1,40 @@
# encoding: utf-8

module Rubocop
module Cop
module Style
# This cop checks for braces in method calls with hash parameters.
class BracesAroundHashParameters < Cop

def on_send(node)
_, method_name, *args = *node
return unless args.length == 1
return if method_name.to_s.end_with?('=')
return if OPERATOR_METHODS.include?(method_name)
arg = args.first
return unless arg && arg.type == :hash && arg.children.any?
has_braces = !! arg.loc.begin
if style == :no_braces && has_braces
convention(arg,
:expression,
'Unnecessary braces around a hash parameter.')
elsif style == :braces && ! has_braces
convention(arg,
:expression,
'Missing braces around a hash parameter.')
end
end

private

def style
case cop_config['EnforcedStyle']
when 'braces' then :braces
when 'no_braces' then :no_braces
else fail 'Unknown style selected!'
end
end
end
end
end
end
4 changes: 2 additions & 2 deletions spec/rubocop/config_loader_spec.rb
Expand Up @@ -240,9 +240,9 @@
' Enabled: true',
])
configuration = load_file
expect(configuration['Encoding']).to eq({
expect(configuration['Encoding']).to eq(
'Enabled' => true
})
)
end
end

Expand Down
207 changes: 207 additions & 0 deletions spec/rubocop/cop/style/braces_around_hash_parameters_spec.rb
@@ -0,0 +1,207 @@
# encoding: utf-8

require 'spec_helper'

describe Rubocop::Cop::Style::BracesAroundHashParameters, :config do
subject(:cop) { described_class.new(config) }

context 'no_braces' do
let(:cop_config) do
{
'EnforcedStyle' => 'no_braces'
}
end

describe 'accepts' do

it 'one non-hash parameter' do
inspect_source(cop, ['where(2)'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one empty hash parameter' do
inspect_source(cop, ['where({})'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one hash parameter with separators' do
inspect_source(cop, ["where( { \n }\t ) "])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'multiple non-hash parameters' do
inspect_source(cop, ['where(1, "2")'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one hash parameter without braces' do
inspect_source(cop, ['where(x: "y")'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one hash parameter without braces and multiple keys' do
inspect_source(cop, ['where(x: "y", foo: "bar")'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one hash parameter without braces and one hash value' do
inspect_source(cop, ['where(x: { "y" => "z" })'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'multiple hash parameters with braces' do
inspect_source(cop, ['where({ x: 1 }, { y: 2 })'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'property assignment with braces' do
inspect_source(cop, ['x.z = { y: "z" }'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'operator with a hash parameter with braces' do
inspect_source(cop, ['x.z - { y: "z" }'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one non-hash parameter followed by a hash parameter with braces' do
inspect_source(cop, ['where(1, { y: 2 })'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonas054 is totally right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

end

describe 'registers an offence for' do

it 'one object method hash parameter with braces' do
inspect_source(cop, ['x.func({ y: "z" })'])
expect(cop.messages).to eq([
'Unnecessary braces around a hash parameter.'
])
expect(cop.highlights).to eq(['{ y: "z" }'])
end

it 'one hash parameter with braces' do
inspect_source(cop, ['where({ x: 1 })'])
expect(cop.messages).to eq([
'Unnecessary braces around a hash parameter.'
])
expect(cop.highlights).to eq(['{ x: 1 }'])
end

it 'one hash parameter with braces and separators' do
inspect_source(cop, ["where( \n { x: 1 } )"])
expect(cop.messages).to eq([
'Unnecessary braces around a hash parameter.'
])
expect(cop.highlights).to eq(['{ x: 1 }'])
end

it 'one hash parameter with braces and multiple keys' do
inspect_source(cop, ['where({ x: 1, foo: "bar" })'])
expect(cop.messages).to eq([
'Unnecessary braces around a hash parameter.'
])
expect(cop.highlights).to eq(['{ x: 1, foo: "bar" }'])
end

end

end

context 'braces' do
let(:cop_config) do
{
'EnforcedStyle' => 'braces'
}
end

describe 'accepts' do

it 'an empty hash parameter' do
inspect_source(cop, ['where({})'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one non-hash parameter' do
inspect_source(cop, ['where(2)'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'multiple non-hash parameters' do
inspect_source(cop, ['where(1, "2")'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one hash parameter with braces' do
inspect_source(cop, ['where({ x: 1 })'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'multiple hash parameters with braces' do
inspect_source(cop, ['where({ x: 1 }, { y: 2 })'])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one hash parameter with braces and spaces around it' do
inspect_source(cop, [
'where( { x: 1 } )'
])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

it 'one hash parameter with braces and separators around it' do
inspect_source(cop, ["where( \t { x: 1 \n } )"])
expect(cop.messages).to be_empty
expect(cop.highlights).to be_empty
end

end

describe 'registers an offence for' do

it 'one hash parameter without braces' do
inspect_source(cop, ['where(x: "y")'])
expect(cop.messages).to eq([
'Missing braces around a hash parameter.'
])
expect(cop.highlights).to eq(['x: "y"'])
end

it 'one hash parameter with multiple keys and without braces' do
inspect_source(cop, ['where(x: "y", foo: "bar")'])
expect(cop.messages).to eq([
'Missing braces around a hash parameter.'
])
expect(cop.highlights).to eq(['x: "y", foo: "bar"'])
end

it 'one hash parameter without braces with one hash value' do
inspect_source(cop, ['where(x: { "y" => "z" })'])
expect(cop.messages).to eq([
'Missing braces around a hash parameter.'
])
expect(cop.highlights).to eq(['x: { "y" => "z" }'])
end

end

end
end