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.

Sign up for GitHub

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

Jump to bottom

Add Style/MethodCallWithArgsParentheses cop #3797

Merged
merged 1 commit into from Jan 3, 2017
Merged

Add Style/MethodCallWithArgsParentheses cop #3797

merged 1 commit into from Jan 3, 2017

Conversation

dominh
Copy link
Contributor

@dominh dominh commented Dec 18, 2016 •
edited

This cop checks presence of parentheses in method calls containing parameters.

Example

# bad
array.delete e

# good
array.delete(e)

I'm not sure if I should merge it with Style/MethodCallParentheses or something?
Should I make this cop enabled by default?

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@dominh dominh force-pushed the method_call_with_args_parentheses branch from 456fe7f to 4eff5e7 Compare December 18, 2016 14:08
@Drenmi
Copy link
Collaborator

Drenmi commented Dec 18, 2016

This is a great start! 😀

I opened an issue for this recently, so you can reference that id in the CHANGELOG and commit message.

Also, I think we should not release this cop without a whitelisting configuration option, so that when people (inevitably) start posting issues about some methods, we can point them to that. Also some methods (like #puts should probably be in there by default. WDYT? 😀

@dominh
Copy link
Contributor Author

dominh commented Dec 18, 2016

@Drenmi Yes, I fully agree with you. Do we have a default list of Method which needs to be ignored?

@Drenmi
Copy link
Collaborator

Drenmi commented Dec 19, 2016 •
edited

@dominh: Unfortunately not. The ones I can think of, off the top of my head are:

  • #p, #puts, #print
  • #require, #require_relative and #load
  • #raise and #fail

These all live in Kernel, and we should match only receiver nil, in case someone implemented these on their own objects.

There are others that are framework specific, like render in Rails, and a bunch of stuff in RSpec, but this is not something RuboCop should know of. Hence the importance for the whitelisting option. 😀

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 19, 2016

I'm not sure if I should merge it with Style/MethodCallParentheses or something?

Probably they should remain separate cops. And we should probably rename the old cop, as it's name is super misleading.

Should I make this cop enabled by default?

Sure. If you make it work properly. ;-)

There were a few attempts to make a cop that knew where parens were needed and when it was ok to omit them, but none of them got far enough to be merged and used.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 26, 2016

Any progress here?

@dominh
Copy link
Contributor Author

dominh commented Dec 27, 2016 •
edited

For now, I'd to stay with this cop disabled by default and I'd to add possibility of ignoring some methods because I haven't any idea what should be ignored by default. In Ruby, we have a lot of DSL and so on which looks better when method calls haven't parentheses. I'm going to look into code tomorrow on the evening. What do you think?

@dominh dominh force-pushed the method_call_with_args_parentheses branch from 4eff5e7 to de8df2a Compare December 29, 2016 17:20
@dominh
Copy link
Contributor Author

dominh commented Dec 29, 2016

I've just update the commit

bbatsov
bbatsov reviewed Jan 2, 2017
View reviewed changes
lib/rubocop/cop/style/method_call_with_args_parentheses.rb
# parameters.
# As in popular Ruby's frameworks a lot of methods should always be
# called without parentheses,
# users can ignore them by passing their names to IgnoredMethods option.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some code examples here.

bbatsov
bbatsov reviewed Jan 2, 2017
View reviewed changes
manual/cops_style.md
As in popular Ruby's frameworks a lot of methods should always be called without parentheses,
users can ignore them by passing their names to IgnoredMethods option.

### Example
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you edit this file manually? I'm surprised to see examples here when there are none in the cop's description.

@dominh dominh force-pushed the method_call_with_args_parentheses branch from de8df2a to d3787d3 Compare January 3, 2017 16:16
@dominh
Copy link
Contributor Author

dominh commented Jan 3, 2017

@bbatsov yeah, it looks like I did these changes manually, strange. Anyway, I've fixed it and updated a commit.

@bbatsov bbatsov merged commit 64f8bd8 into rubocop:master Jan 3, 2017
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 3, 2017

👍

pocke added a commit to pocke/rubocop that referenced this pull request Jan 14, 2017
@pocke
Warn about obsolete cops and parameters
d88d766 
This change improves obsolete warnings in three ways.

1. Show all obsoletes
--------

Currently, RuboCop displays first obsolete warning only.

```yaml
 # two obsolete cops
Style/SingleSpaceBeforeFirstArg:
  Enabled: false
Style/SpaceBeforeModifierKeyword:
  Enabled: false
```

```
Error: The `Style/SingleSpaceBeforeFirstArg` cop has been renamed to `Style/SpaceBeforeFirstArg.
(obsolete configuration found in /tmp/tmp.9T2ers7SW8/.rubocop.yml, please update it)
```

This change makes to display all obsolete warnings.

```
Error: The `Style/SingleSpaceBeforeFirstArg` cop has been renamed to `Style/SpaceBeforeFirstArg.`
(obsolete configuration found in /tmp/tmp.9T2ers7SW8/.rubocop.yml, please update it)
The `Style/SpaceBeforeModifierKeyword` cop has been removed. Please use `Style/SpaceAroundKeyword` instead.
(obsolete configuration found in /tmp/tmp.9T2ers7SW8/.rubocop.yml, please update it)
```

2. Add obsolete cops
---------

`Style/MethodCallParentheses` and `Lint/Eval` are renamed.
- rubocop#3797
- rubocop#3820

So, I added the cops to `OBSOLATE_COPS`.

3. Add obsolete parameters

In rubocop#3765, some cop's parameters are renamed.

So, I added the parameters to `OBSOLATE_PARAMETERS`
@pocke pocke mentioned this pull request Jan 14, 2017
Merged
11 tasks
bbatsov pushed a commit that referenced this pull request Jan 14, 2017
@pocke @bbatsov
Warn about obsolete cops and parameters
a5749d8 
This change improves obsolete warnings in three ways.

1. Show all obsoletes
--------

Currently, RuboCop displays first obsolete warning only.

```yaml
 # two obsolete cops
Style/SingleSpaceBeforeFirstArg:
  Enabled: false
Style/SpaceBeforeModifierKeyword:
  Enabled: false
```

```
Error: The `Style/SingleSpaceBeforeFirstArg` cop has been renamed to `Style/SpaceBeforeFirstArg.
(obsolete configuration found in /tmp/tmp.9T2ers7SW8/.rubocop.yml, please update it)
```

This change makes to display all obsolete warnings.

```
Error: The `Style/SingleSpaceBeforeFirstArg` cop has been renamed to `Style/SpaceBeforeFirstArg.`
(obsolete configuration found in /tmp/tmp.9T2ers7SW8/.rubocop.yml, please update it)
The `Style/SpaceBeforeModifierKeyword` cop has been removed. Please use `Style/SpaceAroundKeyword` instead.
(obsolete configuration found in /tmp/tmp.9T2ers7SW8/.rubocop.yml, please update it)
```

2. Add obsolete cops
---------

`Style/MethodCallParentheses` and `Lint/Eval` are renamed.
- #3797
- #3820

So, I added the cops to `OBSOLATE_COPS`.

3. Add obsolete parameters

In #3765, some cop's parameters are renamed.

So, I added the parameters to `OBSOLATE_PARAMETERS`
@junegunn junegunn mentioned this pull request Apr 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbatsov bbatsov bbatsov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dominh @Drenmi @bbatsov