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

Make Style/AccessModifierDeclarations disabled by default #6066

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented Jul 1, 2018

Follow up of #5953, #6032

I think it is too difficult to enforce group (default) style by default because it doesn't work for some methods.

For example, def_delegator doesn't work when using group style:

require 'forwardable'

class Cat
  def meow
    puts "Meow!"
  end
end

class CatCage
  extend Forwardable

  def initialize(cat)
    @cat = cat
  end

  private

  def_delegator :@cat, :meow
end
[8] pry(main)> CatCage.new(Cat.new).meow
Meow!
=> nil

This is because def_delegator defines a method by self.module_eval. Forcing group styles by default may be misleading for the above problems.

Maybe it is needed to add such methods to the whitelist and exclude them from inspection, but it seems to be difficult for me.

First of all, I think it should make this cop disabled by default and help users confused by this cop. WDYT? @brandonweiss


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).
  • 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.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

Follow up of rubocop#5953, rubocop#6032

I think it is too difficult to enforce `group` (default) style
by default because it doesn't work for some methods.

For example, `def_delegator` doesn't work when using `group` style:

```ruby
require 'forwardable'

class Cat
  def meow
    puts "Meow!"
  end
end

class CatCage
  extend Forwardable

  def initialize(cat)
    @cat = cat
  end

  private

  def_delegator :@cat, :meow
end
```

```
[8] pry(main)> CatCage.new(Cat.new).meow
Meow!
=> nil
```

This is because `def_delegator` defines a method by `self.module_eval`.
Forcing group styles by default may be misleading for the above problems.

Maybe it is needed to add such methods to the whitelist and exclude
them from inspection, but it seems to be difficult for me.

First of all, I think it should make this cop disabled by default
and help users confused by this cop. WDYT? @brandonweiss
@wata727 wata727 force-pushed the disabled_access_modifier_declarations_by_default branch from b7d2588 to 20d50f0 Compare July 1, 2018 06:42
@brandonweiss
Copy link
Contributor

I think it’s probably best to improve the cop, not disable it by default. The RuboCop team has expressed a dislike of cops that are disabled by default (which I agree with) as practically no one goes through and enables them, effectively meaning they might as well not exist.

I think the improvement of adding a whitelist of tokens that accessors can be inlined before while using group configuration probably makes the most sense.

@wata727
Copy link
Contributor Author

wata727 commented Jul 5, 2018

Thanks for your opinion. This approach seems unacceptable to the team, so I will close it once.

@wata727 wata727 closed this Jul 5, 2018
@wata727 wata727 deleted the disabled_access_modifier_declarations_by_default branch July 5, 2018 13:03
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.

None yet

2 participants