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.

Sign up for GitHub

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

Skip calling to_hash for nil #602

Closed
wants to merge 2 commits into from
Closed

Skip calling to_hash for nil #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7c4dfa
Skip calling to_hash for nil
ashkan18 Oct 25, 2017
f04b01c
Add test for not calling to_hash on nil
ashkan18 Oct 27, 2017
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
2 changes: 1 addition & 1 deletion lib/stripe/stripe_object.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def as_json(*a)

def to_hash
maybe_to_hash = lambda do |value|
value.respond_to?(:to_hash) ? value.to_hash : value
value && value.respond_to?(:to_hash) ? value.to_hash : value
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change this to !value.nil? instead of just value to avoid changing the behavior for non-nil falsey values.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks :) I thought about that, the thing is in case of false assertion of this statement we end up just using the value and basically not call to_hash on it which should be fine for any non-nil falsey values but let me know if you can think of any issues 🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

I always forget that false and nil are the only falsey values in Ruby. Should be fine then, as FalseClass does not have a to_hash method anyway!

end

@values.each_with_object({}) do |(key, value), acc|
Expand Down
50 changes: 33 additions & 17 deletions test/stripe/stripe_object_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,23 +115,39 @@ class TestObject < Stripe::StripeObject; end
end
end

should "recursively call to_hash on its values" do
# deep nested hash (when contained in an array) or StripeObject
nested_hash = { id: 7, foo: "bar" }
nested = Stripe::StripeObject.construct_from(nested_hash)

obj = Stripe::StripeObject.construct_from(id: 1,
# simple hash that contains a StripeObject to help us test deep
# recursion
nested: { object: "list", data: [nested] },
list: [nested])

expected_hash = {
id: 1,
nested: { object: "list", data: [nested_hash] },
list: [nested_hash],
}
assert_equal expected_hash, obj.to_hash
context "#to_hash" do
should "skip calling to_hash on nil" do
module NilWithToHash
def to_hash
raise "Can't call to_hash on nil"
end
end
NilClass.include NilWithToHash

hash_with_nil = { id: 3, foo: nil }
obj = StripeObject.construct_from(hash_with_nil)
expected_hash = { id: 3, foo: nil }
assert_equal expected_hash, obj.to_hash
end

should "recursively call to_hash on its values" do
# deep nested hash (when contained in an array) or StripeObject
nested_hash = { id: 7, foo: "bar" }
nested = Stripe::StripeObject.construct_from(nested_hash)

obj = Stripe::StripeObject.construct_from(id: 1,
# simple hash that contains a StripeObject to help us test deep
# recursion
nested: { object: "list", data: [nested] },
list: [nested])

expected_hash = {
id: 1,
nested: { object: "list", data: [nested_hash] },
list: [nested_hash],
}
assert_equal expected_hash, obj.to_hash
end
end

should "assign question mark accessors for booleans" do
Expand Down