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

Support autocorrection even if reject is used on Performance/Count #307

Merged
merged 1 commit into from
Oct 9, 2022

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Oct 1, 2022

Until now, autocorrect was given up when reject was used, but it is now supported.

# bad
array.reject { |e| e > 2 }.size

# good
array.count { |e| !(e > 2) }
# bad
array.reject { |e| e > 2 }.count

# good
array.count { |e| !(e > 2) }
# bad
array.reject { |e| e > 2 }.length

# good
array.count { |e| !(e > 2) }
# bad
array.reject(&:value).size

# good
array.count { |element| !element.value }
# bad
array.reject(&block).size

# good
array.count { !block.call }
# bad
array.reject {
  foo
  bar
}.length

# good
array.count {
  foo
  !(bar)
}

The last one looks a bit weird but I think other cops autocorrects it to some other good form. If there is a better way to negate statements, I'd like to use it.


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.

Comment on lines 308 to 338
array.count {
!(foo
bar)
}
Copy link
Member

@koic koic Oct 5, 2022

Choose a reason for hiding this comment

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

It looks like it needs to be autocorrected to the following for compatible.

Suggested change
array.count {
!(foo
bar)
}
array.count {
foo
!bar
}

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 think that is the point I mentioned at the end of the description, do you have any better ideas to achieve that, and what about this thought?

other cops autocorrects it to some other good form

By the way, you say "for compatible", what exactly is incompatible?

Copy link
Member

Choose a reason for hiding this comment

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

Ah..., it doesn't contribute to compatibility. But ! to foo makes no sense.

Copy link
Contributor Author

@r7kamura r7kamura Oct 5, 2022

Choose a reason for hiding this comment

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

Perhaps the implementation that Solargraph uses to estimate the method's return type might be helpful:

If we have a similar implementation, we can see which nodes can be the return value of a block, and if we negate them, I think we can achieve most of what we want to do. Though this may be a little too heavy implementation for this cop to handle.

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 can be achieved by inverting the last sentence of block with !. Anyway, let's make a better autocorrect separately.

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 changed it so that the only last statement is negated in the case of begin node as a simple enough approach. I think indeed it's going to work there, even if it's not a perfect autocorrection.

@koic koic merged commit 3dd06b5 into rubocop:master Oct 9, 2022
@koic
Copy link
Member

koic commented Oct 9, 2022

Thanks!

@r7kamura r7kamura deleted the feature/count branch October 9, 2022 20:33
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.

2 participants