Add find_by #127

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@fdhadzh
Contributor

fdhadzh commented Feb 13, 2015

No description provided.

lib/no_brainer/criteria/find_by.rb
+ extend ActiveSupport::Concern
+
+ def find_by(*block)
+ where(*block).first

This comment has been minimized.

@nviennot

nviennot Feb 13, 2015

Owner

use (*args, &block) instead of (*block)

@nviennot

nviennot Feb 13, 2015

Owner

use (*args, &block) instead of (*block)

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Feb 13, 2015

Owner

rename find_by -> find_by? and find_by! -> find_by. So we are consistent with the rest of the API (find raises, find? doesn't)

That's it :)
And thank you :)

Owner

nviennot commented Feb 13, 2015

rename find_by -> find_by? and find_by! -> find_by. So we are consistent with the rest of the API (find raises, find? doesn't)

That's it :)
And thank you :)

@fdhadzh

This comment has been minimized.

Show comment
Hide comment
@fdhadzh

fdhadzh Feb 13, 2015

Contributor

Success

Contributor

fdhadzh commented Feb 13, 2015

Success

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Feb 13, 2015

Owner

The current implementation of find is:

  • Model.find(id) is equivalent to Model.unscoped.where(:id => id).first!.
  • Model.find?(id) is equivalent to Model.unscoped.where(:id => id).first.

Note that find() is only defined on the Model class, and not on criteria.

I'd like to have find and find_by semantics to be similar. So I might have to remove the unscoped semantic. I'll sleep on it. If you have any opinion on what should be done, and why, I'd be happy to hear it.

Owner

nviennot commented Feb 13, 2015

The current implementation of find is:

  • Model.find(id) is equivalent to Model.unscoped.where(:id => id).first!.
  • Model.find?(id) is equivalent to Model.unscoped.where(:id => id).first.

Note that find() is only defined on the Model class, and not on criteria.

I'd like to have find and find_by semantics to be similar. So I might have to remove the unscoped semantic. I'll sleep on it. If you have any opinion on what should be done, and why, I'd be happy to hear it.

@fdhadzh

This comment has been minimized.

Show comment
Hide comment
@fdhadzh

fdhadzh Feb 13, 2015

Contributor

Maybe i can add find_by and find_by? to criteria with your find function?

Contributor

fdhadzh commented Feb 13, 2015

Maybe i can add find_by and find_by? to criteria with your find function?

@fdhadzh fdhadzh changed the title from Add find_by to Add `find_by` Feb 13, 2015

@fdhadzh fdhadzh changed the title from Add `find_by` to Add find_by Feb 13, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Feb 13, 2015

Owner

I'll deal with it.

Owner

nviennot commented Feb 13, 2015

I'll deal with it.

@fdhadzh fdhadzh closed this Feb 13, 2015

@fdhadzh fdhadzh reopened this Feb 13, 2015

@fdhadzh fdhadzh closed this Feb 13, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Feb 13, 2015

Owner

Currenty, find is implemented with RQL get(). To do this, we can't use the default_scope or apply find on a criteria.
To use find on a criteria, it means that we need to use get_all(). However, from my benchmark, it appears that it's 50% slower.
So that mean that to have find to match the semantics of find_by, we'll have to give up 50% of the speed of the fast path Model.find(id). I'm not so thrilled about it :/

Owner

nviennot commented Feb 13, 2015

Currenty, find is implemented with RQL get(). To do this, we can't use the default_scope or apply find on a criteria.
To use find on a criteria, it means that we need to use get_all(). However, from my benchmark, it appears that it's 50% slower.
So that mean that to have find to match the semantics of find_by, we'll have to give up 50% of the speed of the fast path Model.find(id). I'm not so thrilled about it :/

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Feb 13, 2015

Owner

hahahaha (seeing the image :D)

Owner

nviennot commented Feb 13, 2015

hahahaha (seeing the image :D)

nviennot added a commit that referenced this pull request Feb 13, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Feb 13, 2015

Owner

Merged it in, changed the behavior of find().

Owner

nviennot commented Feb 13, 2015

Merged it in, changed the behavior of find().

@kuldeepaggarwal

This comment has been minimized.

Show comment
Hide comment
@kuldeepaggarwal

kuldeepaggarwal Jul 20, 2015

It is inconsistence with Rails ActiveRecord API, I think we should develop the public API as same like Rails.

It is inconsistence with Rails ActiveRecord API, I think we should develop the public API as same like Rails.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jul 20, 2015

Owner

We settled on removing it: #145

find_by() does not raise with AR, but find() does. I'm not too happy about introducing inconsistent API.

Owner

nviennot commented Jul 20, 2015

We settled on removing it: #145

find_by() does not raise with AR, but find() does. I'm not too happy about introducing inconsistent API.

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