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

CompactBlank cop raising an offense when using delete_if #650

Closed
Benaaaaa opened this issue Mar 7, 2022 · 2 comments · Fixed by #651
Closed

CompactBlank cop raising an offense when using delete_if #650

Benaaaaa opened this issue Mar 7, 2022 · 2 comments · Fixed by #651
Labels
enhancement New feature or request

Comments

@Benaaaaa
Copy link

Benaaaaa commented Mar 7, 2022

Currently there is an existing cop that tells us to use compact_blank, compact_blank! instead of reject, reject! respectively when deleting blanks.
There is another option to be used and that is delete_if. So instead of doing this:
{ a: 'a', b: 'b', c: 'c', d: {}, e: {}, f: [] }.delete_if{ |_k, v| v.blank? }
The cop should recommend doing this:
{ a: 'a', b: 'b', c: 'c', d: {}, e: {}, f: [] }.compact_blank!
It works virtually the same as reject! but is not considered in the cop. I have run some local bechmarking to compare reject! and delete_if to see if maybe one should be favored over the other:

    n = 5_000_000
    Benchmark.bm do |x|
      x.report('delete_if') { n.times { { a: 'a', b: 'b', c: 'c', d: {}, e: {}, f: [] }.delete_if { |_k, v| v.blank? } } }
      x.report('reject!') { n.times { { a: 'a', b: 'b', c: 'c', d: {}, e: {}, f: [] }.reject!{ |_k, v|  v.blank? } } }
    end

This is the end result:

       user        system      total        real
  delete_if       5.963637   0.054312   6.017949 (  6.208843)
  reject!         5.812006   0.049361   5.861367 (  6.132459)

As you can see the run time is almost the same ( even when repeating the proccess 10 times the numbers were very similar)
And so because of the results I recommend adding the situation where using delete_if should raise an offense as well.
If need be I could work on the cop myself

This is the output of rubocop -V:

  • rubocop-infinum 0.6.0
  • rubocop-performance 1.13.2
  • rubocop-rails 2.13.2
  • rubocop-rspec 2.8.0
@koic
Copy link
Member

koic commented Mar 7, 2022

Thank you for the feedback!
delete_if always returns self, but reject! returns self if one or more elements are deleted, nil if no element is deleted. So they have different behaviors.

And compact_blank! has different implementations for ActionController::Parameters, Array, and Hash. If the cop makes a mistake, auto-corrected code may get unexpected behavior.

I'll take the issue to update the cop implementation and documentation.

@koic koic added the enhancement New feature or request label Mar 7, 2022
koic added a commit to koic/rubocop-rails that referenced this issue Mar 7, 2022
…nk)`

Fixes rubocop#650.

This PR makes `Rails/CompactBlank` aware of `delete_if(&:blank)`.

It is unsafe because `compact_blank!` has different implementations for `Array`, `Hash`,
and `ActionController::Parameters`.
`Array#compact_blank!`, `Hash#compact_blank!` are equivalent to `delete_if(&:blank?)`.
`ActionController::Parameters#compact_blank!` is equivalent to `reject!(&:blank?)`.
If the cop makes a mistake, auto-corrected code may get unexpected behavior.

This PR also updates unsafe document about it.
@Benaaaaa
Copy link
Author

Benaaaaa commented Mar 8, 2022

Oh, thanks for this explanation, I had no idea that's the difference in behavior. Great work with dealing the problem so fast 👍

@koic koic closed this as completed in #651 Mar 8, 2022
koic added a commit that referenced this issue Mar 8, 2022
…eject_delete_if

[Fix #650] Make `Rails/CompactBlank` aware of `delete_if(&:blank)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants