find_by does not behave like ActiveRecord #145

Closed
sbkok opened this Issue May 21, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@sbkok

sbkok commented May 21, 2015

In ActiveRecord, the find_by method will try to look-up the model, but returns nil when not found.
If you want it to raise exceptions, you should use find_by! instead.
See: RoR Guide on Active Record Querying

However, in NoBrainer, the find_by raises exceptions in case the model was not found.
I believe that the default behaviour of find_by should be changed to ActiveRecord style, such that most gems and other projects do not require specific find implementations to support NoBrainer.

Current implementation: lib/no_brainer/criteria/find.rb - find_by method

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot May 21, 2015

Owner

well, right now we have the nice parallelism where find(id) is find_by(:id => id) and find? uses find_by?.
If find raises, and find_by does not, I'd think it would be confusing. I'd rather remove the find_by API at this point if it creates issues.

Owner

nviennot commented May 21, 2015

well, right now we have the nice parallelism where find(id) is find_by(:id => id) and find? uses find_by?.
If find raises, and find_by does not, I'd think it would be confusing. I'd rather remove the find_by API at this point if it creates issues.

@nviennot nviennot closed this in 92f9d68 May 21, 2015

@sbkok

This comment has been minimized.

Show comment
Hide comment
@sbkok

sbkok May 21, 2015

I agree that the functionality in ActiveRecord is not what you would expect, I'm afraid they will not change this any time soon.
Removing find_by would break the compatibility with ActiveRecord even more though.

sbkok commented May 21, 2015

I agree that the functionality in ActiveRecord is not what you would expect, I'm afraid they will not change this any time soon.
Removing find_by would break the compatibility with ActiveRecord even more though.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot May 21, 2015

Owner

I think it's okay to replace find_by(...) with where(...).first (it's just 4 more characters).
Removing find_by won't be creating unexpected bugs in production. Changing the semantics would.

If some library really needs it, we'll see about reintroducing it. Meanwhile, I see no reason to keep it.

Owner

nviennot commented May 21, 2015

I think it's okay to replace find_by(...) with where(...).first (it's just 4 more characters).
Removing find_by won't be creating unexpected bugs in production. Changing the semantics would.

If some library really needs it, we'll see about reintroducing it. Meanwhile, I see no reason to keep it.

@nviennot nviennot referenced this issue Jul 20, 2015

Closed

Add find_by #127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment