-
-
Notifications
You must be signed in to change notification settings - Fork 190
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
Fix Undefined method `key?' for nil:NilClass error. #672
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this! I need to clarify one thing before this can be merged, please see my question in the review.
@@ -70,6 +70,8 @@ def key?(key, hash = data) | |||
return result | |||
elsif e.is_a?(Symbol) && a.is_a?(Array) | |||
return false | |||
elsif a.is_a?(String) || a.nil? | |||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a.is_a?(String)
was needed? It shouldn't be, at this point all keys should be symbolized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It for the case when root value is a string. I have checked and it is not symbolized.
It covered by this test case
let(:data) do
{
name: "Jane",
address: {city: "Paris", geo: {lat: 1, lon: 2}},
phones: [123, 431],
billing_address: nil,
card: ""
}
end
it "returns false when a nested key is not present and root key is string" do
expect(values.key?([:card, :not_here])).to be(false)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should check for valid types instead?
elsif !(a.is_a?(Array) || a.is_a?(Hash))
return false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexxty7 aahh that's right, this can happen! I think it should be handled separately though, so:
# ...
elsif a.nil?
return false
elsif a.is_a?(String)
return false
# ...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@solnic updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
hello,
result
at the moment I have it solved by:
But it's definitely a trick. |
Fixes #670