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

Add autocorrect support to Style/AccessModifierDeclarations #10966

Merged
merged 1 commit into from Aug 26, 2022

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Aug 26, 2022

I would like to change Style/AccessModifierDeclarations to be able to autocorrect.

I made a little effort about indentation and empty lines with RuboCop::Cop::RangeHelp#range_by_whole_lines and some other treatments, but it does not handle them perfectly. They should be autocorrected by other Layout cops.


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.

@r7kamura r7kamura force-pushed the feature/modifier-autocorrect branch 2 times, most recently from 93c9181 to 2a5e50b Compare August 26, 2022 02:33
Comment on lines 174 to 176
node.parent.children.find do |child|
child&.send_type? && child&.method?(node.method_name) && child&.arguments&.empty?
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
node.parent.children.find do |child|
child&.send_type? && child&.method?(node.method_name) && child&.arguments&.empty?
end
node.parent.children.compact.find do |child|
child.send_type? && child.method?(node.method_name) && child.arguments.empty?
end

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 thought that it would be wasteful to iterate twice on #children for this purpose.
By the way, if we want to remove this safe navigation operators, I think it is better to use #each_child_node than to use #compact, what do you think?

Suggested change
node.parent.children.find do |child|
child&.send_type? && child&.method?(node.method_name) && child&.arguments&.empty?
end
node.parent.each_child_node.find do |child|
child.send_type? && child.method?(node.method_name) && child.arguments.empty?
end

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good choice.

Comment on lines 165 to 167
node.parent.children.find do |child|
child&.def_type? && child&.method?(method_name)
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
node.parent.children.find do |child|
child&.def_type? && child&.method?(method_name)
end
node.parent.children.compact.find do |child|
child.def_type? && child.method?(method_name)
end

@koic
Copy link
Member

koic commented Aug 26, 2022

Could you add expect_correction to the existing expect_offense in this cop's spec?

@r7kamura r7kamura force-pushed the feature/modifier-autocorrect branch from b96160b to f64b7e1 Compare August 26, 2022 03:11
@r7kamura
Copy link
Contributor Author

I added these 2 changes:

  1. Replace #children with #each_child_node
  2. Add expect_correction and expect_no_corrections

Whether or not to explicitly write expect_no_corrections is debatable, but I thought it should be written here because I also want to make it clear that autocorrection is not supported in that situation.

Comment on lines 165 to 166
node.parent.each_child_node.find do |child|
child.def_type? && child.method?(method_name)
Copy link
Member

Choose a reason for hiding this comment

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

each_child_node can filter node types.

Suggested change
node.parent.each_child_node.find do |child|
child.def_type? && child.method?(method_name)
node.parent.each_child_node(:def).find do |child|
child.method?(method_name)

Comment on lines 174 to 175
node.parent.each_child_node.find do |child|
child.send_type? && child.method?(node.method_name) && child.arguments.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
node.parent.each_child_node.find do |child|
child.send_type? && child.method?(node.method_name) && child.arguments.empty?
node.parent.each_child_node(:send).find do |child|
child.method?(node.method_name) && child.arguments.empty?

@r7kamura r7kamura force-pushed the feature/modifier-autocorrect branch from 501af86 to a2fe5ab Compare August 26, 2022 03:59
@koic koic merged commit f70fb12 into rubocop:master Aug 26, 2022
@koic
Copy link
Member

koic commented Aug 26, 2022

Thanks!

@r7kamura r7kamura deleted the feature/modifier-autocorrect branch August 26, 2022 21: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