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 Rule::Base.autocorrect_incompatible_with #534

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FnControlOption
Copy link
Contributor

@FnControlOption FnControlOption commented Jan 5, 2025

Fixes #489

The issue here is that the autocorrections for Style/VerboseBlock and Performance/MinMaxAfterMap are overlapping.

For Performance/MinMaxAfterMap, we (1) replace map with max_of and (2) remove .max.

list.map { |i| i.size }.max
     ^^^                ^^^
     (1)                (2)

(1) is overwritten by Style/VerboseBlock, which replaces map { |i| i.size } with map(&.size).

list.map { |i| i.size }.max
     ^^^^^^^^^^^^^^^^^^ ^^^
        map(&.size)     (2)

As a result, we get

list.map(&.size)

My proposed fix is to run incompatible autocorrections separately instead of all at once.

So first we'll correct Performance/MinMaxAfterMap

list.map { |i| i.size }.max
     ^^^                ^^^
     (1)                (2)

to get

list.max_of { |i| i.size }

Then, we'll correct Style/VerboseBlock to get

list.max_of(&:size)

@Sija
Copy link
Member

Sija commented Jan 6, 2025

Thanks for the quick fix! ❤️ Regarding the implementation I'm afraid it's not scalable, since it'd require analyzing all of the rules in order to compile a matrix of dependent (and incompatible) rules. I was thinking of doing that automatically, as it should be possible given that the corrector knows the positions and sizes of the applied corrections.

@Sija Sija changed the title Add Rule::Base.autocorrect_incompatible_with Add Rule::Base.autocorrect_incompatible_with Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The max after the map is removed
2 participants