Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Remove find_by() due to unclear semantics
Fixes #145
  • Loading branch information
nviennot committed May 21, 2015
1 parent 8a88a4f commit 92f9d68
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 45 deletions.
15 changes: 6 additions & 9 deletions lib/no_brainer/criteria/find.rb
@@ -1,27 +1,24 @@
module NoBrainer::Criteria::Find
extend ActiveSupport::Concern

def find_by?(*args, &block)
where(*args, &block).first
end

def find_by(*args, &block)
find_by?(*args, &block).tap { |doc| raise_not_found(args) unless doc }
raise "find_by() has unclear semantics. Please use where().first instead"
end
alias_method :find_by!, :find_by
alias_method :find_by?, :find_by

def find?(pk)
without_ordering.find_by?(model.pk_name => pk)
without_ordering.where(model.pk_name => pk).first
end

def find(pk)
without_ordering.find_by(model.pk_name => pk)
find?(pk).tap { |doc| raise_not_found(pk) unless doc }

This comment has been minimized.

Copy link
@kuldeepaggarwal

kuldeepaggarwal Jul 20, 2015

I think we can directly write find?(pk) || raise_not_found(pk). wdyt?

This comment has been minimized.

Copy link
@nviennot

nviennot Jul 20, 2015

Author Collaborator

sure: bbb452a

end
alias_method :find!, :find

private

def raise_not_found(args)
raise NoBrainer::Error::DocumentNotFound, "#{model} #{args.inspect.gsub(/\[{(.*)}\]/, '\1')} not found"
def raise_not_found(pk)
raise NoBrainer::Error::DocumentNotFound, "#{model} :#{model.pk_name}=>#{pk.inspect} not found"
end
end
39 changes: 3 additions & 36 deletions spec/integration/criteria/find_spec.rb
Expand Up @@ -2,43 +2,10 @@

describe 'find_by' do
before { load_simple_document }
let!(:doc) { SimpleDocument.create(:field1 => 'apple', :field2 => 'orange') }

context 'when using find_by' do
it 'finds the document' do
SimpleDocument.find_by?(:field1 => 'apple').field2.should == doc.field2
end
end

context 'when passing a field that does not exist' do
it 'raises when field is not exists' do
expect { SimpleDocument.find_by?(:apple => 'field1') }.to raise_error(NoBrainer::Error::UnknownAttribute)
end
end

context 'when no match is found' do
it 'returns nil with find_by?' do
SimpleDocument.find_by?(:field1 => 'anything').should == nil
end

it 'raises with find_by' do
expect { SimpleDocument.find_by(:field1 => 'anything') }
.to raise_error(NoBrainer::Error::DocumentNotFound, /SimpleDocument :field1=>"anything" not found/)
end

it 'raises with find_by!' do
expect { SimpleDocument.find_by!(:field1 => 'anything') }
.to raise_error(NoBrainer::Error::DocumentNotFound, /SimpleDocument :field1=>"anything" not found/)
end
end

context 'when applying a criteria' do
let!(:doc2) { SimpleDocument.create(:field1 => 'apple', :field2 => 'kiwi') }

it 'applies the criteria' do
SimpleDocument.where(:field2 => 'kiwi').find_by(:field1 => 'apple').field2.should == 'kiwi'
SimpleDocument.where(:field2 => 'orange').find_by(:field1 => 'apple').field2.should == 'orange'
end
it 'raises' do
expect { SimpleDocument.find_by(:field1 => 'anything') }
.to raise_error(/unclear semantics/)
end
end

Expand Down

0 comments on commit 92f9d68

Please sign in to comment.