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

Inline in_batches false-positive on Migration/BatchInBatches #14

Closed
barak-neeman opened this issue Jan 26, 2024 · 5 comments · Fixed by #16
Closed

Inline in_batches false-positive on Migration/BatchInBatches #14

barak-neeman opened this issue Jan 26, 2024 · 5 comments · Fixed by #16

Comments

@barak-neeman
Copy link

barak-neeman commented Jan 26, 2024

Model.where(field: nil).in_batches.update_all(field: false)

Gets flagged by the Migration/BatchInBatches cop, and auto-corrected to:

Model.where(field: nil).in_batches.in_batches do |relation|
  relation.update_all(field: false)
end

Please update the Migration/BatchInBatches cop to allow for the above inline usage of in_batches, as the correction is unnecessary and produces code with two consecutive in_batches calls.

@r7kamura
Copy link
Owner

Thank you for your report! I completely forgot about that when I implemented it.

Maybe if we improve in these lines, I think it will work also in inline calls:

def wrong?(node)
batch_processing?(node) &&
!within_in_batches?(node)
end

@r7kamura
Copy link
Owner

Maybe we should also use the inline style for autocorrection?

# bad
User.update_all(some_column: 'some value')

# good
User.in_batches.update_all(some_column: 'some value')

@r7kamura r7kamura changed the title Feature Request: Add support for inline in_batches (Migration/BatchInBatches) Inline in_batches false-positive on Migration/BatchInBatches Jan 27, 2024
@r7kamura
Copy link
Owner

r7kamura commented Jan 27, 2024

Added support for inline in_batches at:

and also changed its autocorrection result to prefer the inline style over the block style at:

@r7kamura
Copy link
Owner

Just published v0.5.0:

It was a good opportunity to improve Migration/BatchInBatches. Thanks again!

@barak-neeman
Copy link
Author

Update works for all my test cases, and amazingly fast too!
Thank you very much!!

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 a pull request may close this issue.

2 participants