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

RedundantSort rule not working properly in case of max_by(&:size) #10061

Closed
Mifrill opened this issue Aug 31, 2021 · 1 comment · Fixed by #10062
Closed

RedundantSort rule not working properly in case of max_by(&:size) #10061

Mifrill opened this issue Aug 31, 2021 · 1 comment · Fixed by #10062
Labels

Comments

@Mifrill
Copy link

Mifrill commented Aug 31, 2021

The goal:

return the last of most sized words in the string

A possible way to solve it:

"hello world".split(/\W+/).sort_by(&:size).last
# => "world"

This code raise rubocop offense:

RuboCop: Use `max_by` instead of `sort_by...last`. [Style/RedundantSort]

If we apply the suggested replacement, then we got a different result:

"hello world".split(/\W+/).max_by(&:size)
# => "hello"

Expected behavior

No suggest replacement when the result going to change.

Actual behavior

Suggest replacement according to Style/RedundantSort rule with result changes.

RuboCop version

1.18.4 (using Parser 3.0.2.0, rubocop-ast 1.9.0, running on ruby 3.0.2 x86_64-linux)
  - rubocop-rails 2.11.3
  - rubocop-rspec 2.4.0
@koic koic added the bug label Aug 31, 2021
koic added a commit to koic/rubocop that referenced this issue Sep 1, 2021
Fixes rubocop#10061.

This PR fixes a false positive for `Style/RedundantSort`
when using `size` method in the block.
bbatsov pushed a commit that referenced this issue Sep 1, 2021
Fixes #10061.

This PR fixes a false positive for `Style/RedundantSort`
when using `size` method in the block.
@dvandersluis
Copy link
Member

dvandersluis commented Sep 23, 2021

Isn't this a problem in general when there are multiple values that translate to the same value? Even without size, sort_by.last will take the last one and max_by will take the first one when there are multiple same values.

I think this cop needs to be marked as unsafe. I opened #10122 for that.

dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Sep 27, 2021
These changes added an exception for `size` but the same issue can occur regardless of what method is used, as long as it results in multiple values having the same resulting value. Instead, this cop is being made unsafe.
bbatsov pushed a commit that referenced this issue Sep 28, 2021
These changes added an exception for `size` but the same issue can occur regardless of what method is used, as long as it results in multiple values having the same resulting value. Instead, this cop is being made unsafe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants