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

Potential semantic change by Rails/FindEach cop #2313

Closed
bootleq opened this issue Oct 9, 2015 · 2 comments
Closed

Potential semantic change by Rails/FindEach cop #2313

bootleq opened this issue Oct 9, 2015 · 2 comments

Comments

@bootleq
Copy link

bootleq commented Oct 9, 2015

Rails/FindEach cop suggests to change

User.where(name: name).each { nil }

to

User.where(name: name).find_each { nil }

which might cause unexpected semantic change:

  1. each {...} returns Array, while find_each {...} returns nil in this case.
  2. find_each does not respect limit and order conditions.
@alexdowad
Copy link
Contributor

I think using obj.each if you actually want an Array is poor coding style. It's not clear that you are doing that to get an array.

However, if the other contributors disagree, I do have code which can "fix" this.

The point about limit and order is a good one.

@alexdowad
Copy link
Contributor

Just opened a PR which fixes this point regarding order and limit. I read on an Internet forum that find_each also ignores select(:field1, :field2, ...), so I included that too.

The point about the return value is (deliberately) not fixed.

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

No branches or pull requests

2 participants