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

Check for parentheses when within parameter list #2426

Closed
agrimm opened this issue Nov 13, 2015 · 7 comments
Closed

Check for parentheses when within parameter list #2426

agrimm opened this issue Nov 13, 2015 · 7 comments

Comments

@agrimm
Copy link
Contributor

agrimm commented Nov 13, 2015

The following code doesn't create an offense:

# Does fooing
class Foo
  def needs_parentheses
    foo(bar 1, 2)
  end
end

Should it do so? I think it's too easy to mistake it as saying foo(bar, 1, 2).

@alexdowad
Copy link
Contributor

I think whatever rules as regards parenthesizing method calls are applied elsewhere, should also be applied here in just the same way. It doesn't deserve a special case.

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 14, 2015

There are very few rules that are enforced automatically about parens. Mostly because it's really hard to determine if something is a DSL method (which generally are invoked without parens).

I think @agrimm is right that the code he presents in the example is a bit unclear.

@jonas054
Copy link
Collaborator

I think a new cop for this special case would be a good idea. A method call without parentheses that's an argument in another method call is a weird thing.

@alexdowad
Copy link
Contributor

I have a cop for this, waiting for my outstanding PR to be merged before I open another one.

@alexdowad
Copy link
Contributor

The new cop flags this:

 expect(slice.any? { |l| l.include? cop.cop_name }).to be_truthy

...because of the #include? call. Do you like this? Or should this be exempt, perhaps because it is inside a block?

@BenMorganIO
Copy link

Hmmm I'm thinking this was a bit too hastey. I got some code that looks like:

foo(bar 'hello')

I consider that quite OK since you can tell there's an inside function. I'm thinking that this cop should only be on if the arity is greater than 1.

@alexdowad
Copy link
Contributor

I consider that quite OK since you can tell there's an inside function. I'm thinking that this cop should only be on if the arity is greater than 1.

Rather than adding more and more rules about when parens are required, I suggest we keep this cop simple. People who don't like the simple rule can disable the cop.

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

No branches or pull requests

5 participants