[Fix #12242] Support AllowModifiersOnAttrs option for Style/AccessModifierDeclarations

[krororo]

Fixes #12242

This PR supports AllowModifiersOnAttrs options for Style/AccessModifierDeclarations.

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• 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.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[bbatsov]

Hmm, I'm a bit puzzled by the proposal as I imagine that almost everyone will prefer to use the inline style in this case. Is there something I'm missing here?

[krororo]

I agree that some people prefer the inline style. However, as noted in the issue, there is a case where we want to keep the group style and write only attr_* in inline style.

For example, in applications that have been maintained for a long time, much of the code is already written in the group style. Changing the setting to inline style requires modifying a lot of code. Also, developers who are not very familiar with Ruby may know that the modifier applies to def, but they may not know that it also applies to attr_*. It would be easier to understand if we could write attr_* in inline style.

This option is similar to the Style/AllowModifiersOnSymbols option that is enabled by default. So we could write the code as follows, but it would be simpler to write the attr_* in inline style.

class Foo
  attr_reader :a
  private :a
  attr_accessor :b
  private :b, :b=
end

[krororo]

I wondered what the default value should be, but decided to set it to false for compatibility.

[ntkme]

Set it to true won't cause any existing code to suddenly have a violation, because this is not really a new rule but just an exception to an existing rule. So I think it can default to true to align with AllowModifiersOnSymbols.

[krororo]

Indeed. Updated.

[ntkme]

Also :attr (https://ruby-doc.org/3.2.2/Module.html#method-i-attr)

Suggested change

	(send nil? {:attr_reader :attr_writer :attr_accessor} _))
	(send nil? {:attr :attr_reader :attr_writer :attr_accessor} _))

[krororo]

Good catch! I forgot it.

[krororo]

Could you give us an update? Can I do something to merge this PR?

[bbatsov]

@krororo Sorry for dropping the ball on this! Too much focus on Prism support recently.

For example, in applications that have been maintained for a long time, much of the code is already written in the group style. Changing the setting to inline style requires modifying a lot of code. Also, developers who are not very familiar with Ruby may know that the modifier applies to def, but they may not know that it also applies to attr_. It would be easier to understand if we could write attr_ in inline style.

Makes sense to me. Just reflect this also it cop's documentation, so it's clearer for people why this option exists.

[bbatsov]

I think those examples should be extended to feature something else besides attributes, so people would understand the idea is to augment whatever the primary enforced style of the cop is.

[krororo]

I would like to confirm that my understanding is correct, should I add method definitions like this?

# good
class Foo

  public attr_reader :bar
  protected attr_writer :baz
  private attr_accessor :qux
  private attr :quux

  def public_method; end

  private

  def private_method; end

end

[bbatsov]

Yep.

[krororo]

@bbatsov No problem. Thank you for the review.

Makes sense to me. Just reflect this also it cop's documentation, so it's clearer for people why this option exists.

OK. I will update cop's documentation.

[krororo]

@bbatsov Updated cop's documentation and rebased on top of the current master branch. Could you please review again?