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

Teach #from_hash to raise TypeError when argument is not a Hash #203

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/representable/hash.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def collection_representer_class

def from_hash(data, options={}, binding_builder=Binding)
data = filter_wrap(data, options)
raise TypeError, "Expected Hash, got #{data.class}." unless ::Hash === data
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apotonick could rely on duck typing here (unless data.respond_to?(:has_key?), but I think this approach is probably safer.


update_properties_from(data, options, binding_builder)
end
Expand Down
8 changes: 8 additions & 0 deletions lib/representable/hash/collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ def items(options={}, &block)

# TODO: revise lonely collection and build separate pipeline where we just use Serialize, etc.

def from_hash(data, options={}, binding_builder=Binding)
data = filter_wrap(data, options)
data = [data] if ::Hash === data
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apotonick this makes #from_hash more lenient - accepting a single Hash for a collection. I'm not sure if this is desirable.

raise TypeError, "Expected Enumerable, got #{data.class}." unless data.respond_to?(:each)

update_properties_from(data, options, binding_builder)
end

def create_representation_with(doc, options, format)
options = normalize_options(options)
options[:_self] = options
Expand Down
36 changes: 34 additions & 2 deletions test/for_collection_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,38 @@ module SongRepresenter
it { parse(representer.for_collection.new([]), input).must_equal songs }
end
end

# with module including module
end

describe 'bad collections' do
let(:representer) {
Class.new(Representable::Decorator) do
include Representable::Hash
property :name

collection_representer class: Song
end
}

it 'parses when argument is an Array' do
representer.for_collection.new([]).from_hash([{ 'name' => 'Gateways' }])
.must_equal [Song.new('Gateways')]
end

it 'parses when argument is a Hash' do
representer.for_collection.new([]).from_hash('name' => 'Mother North')
.must_equal [Song.new('Mother North')]
end

it 'raises a TypeError when argument is not an Enumerable' do
from_hash = -> {
representer.for_collection.new([]).from_hash(nil)
}.must_raise TypeError
from_hash.message.must_equal 'Expected Enumerable, got NilClass.'

from_hash = -> {
representer.for_collection.new([]).from_hash('')
}.must_raise TypeError
from_hash.message.must_equal 'Expected Enumerable, got String.'
end
end
end
9 changes: 8 additions & 1 deletion test/hash_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ class HashWithScalarPropertyTest < MiniTest::Spec
album.title.must_equal nil
album.extend(representer).from_hash("title" => nil)
end

it 'raises a TypeError when argument is not a Hash' do
from_hash = -> {
album.extend(representer).from_hash('not Hashish')
}.must_raise TypeError
from_hash.message.must_equal 'Expected Hash, got String.'
end
end
end

Expand Down Expand Up @@ -157,4 +164,4 @@ class HashWithScalarCollectionTest < MiniTest::Spec
album.extend(representer).from_hash("songs" => ["Off Key Melody", "Sinking"]).must_equal Album.new(["Off Key Melody", "Sinking"])
end
end
end
end
10 changes: 7 additions & 3 deletions test/wrap_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,15 @@ class BandDecorator < Representable::Decorator

it do
band.from_hash({"bands" => {"name"=>"Social Distortion"}}).name.must_equal "Social Distortion"
band.from_hash({"name"=>"Social Distortion"}, wrap: false).name.must_equal "Social Distortion"
band.from_hash({band: {"name"=>"Social Distortion"}}, wrap: :band).name.must_equal "Social Distortion"
band.from_hash({"name"=>"Bad Religion"}, wrap: false).name.must_equal "Bad Religion"
band.from_hash({"band"=>{"name"=>"Pennywise"}}, wrap: :band).name.must_equal "Pennywise"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apotonick as long as filter_wrap_for calls #to_s on wrap (and we don't mix in AllowSymbols), this is necessary.

end

it 'raises a TypeError when unwrapped argument is not a Hash' do
-> { band.from_hash('bands' => '') }.must_raise TypeError
-> { band.from_hash('', wrap: false) }.must_raise TypeError
-> { band.from_hash({'band' => ''}, wrap: :band) }.must_raise TypeError
end

class AlbumDecorator < Representable::Decorator
include Representable::Hash
Expand Down Expand Up @@ -149,4 +154,3 @@ class AlbumDecorator < Representable::Decorator
# album.from_hash({"albums" => {"band" => {"name"=>"Rvivr"}}}).band.name.must_equal "Rvivr"
# end
end