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

Conversation

dblock
Copy link
Contributor

@dblock dblock commented Oct 8, 2013

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
Copy link

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 })

@@ -184,3 +184,6 @@ TrivialAccessors:
VariableName:
# Valid values are: snake_case, camelCase
EnforcedStyle: snake_case

BracesInHashParameters:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@jonas054
Copy link
Collaborator

jonas054 commented Oct 8, 2013

@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
Copy link
Contributor Author

dblock commented Oct 8, 2013

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

end

it 'accepts multiple hash parameters' do
inspect_source(cop, ['where(x: "y", foo: "bar")'])
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 actually just one hash with multiple keys, so the heading "accepts multiple hash parameters" is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewed and updated all names.

@jonas054
Copy link
Collaborator

jonas054 commented Oct 9, 2013

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
Copy link
Contributor Author

dblock commented Oct 9, 2013

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

@dblock
Copy link
Contributor Author

dblock commented Oct 9, 2013

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

@dblock
Copy link
Contributor Author

dblock commented Oct 9, 2013

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

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 9, 2013

@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
Copy link
Contributor Author

dblock commented Oct 9, 2013

Here're you go.

bbatsov added a commit that referenced this pull request Oct 9, 2013
New cop  checks for braces in function calls with hash arguments.
@bbatsov bbatsov merged commit ef8f766 into rubocop:master Oct 9, 2013
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 9, 2013

Thanks!

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.

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

Successfully merging this pull request may close these issues.

4 participants