don't downcast when assigning to a field_array #252
Conversation
|
addresses #251 |
| @@ -9,7 +9,7 @@ class MessageField < BaseField | |||
| # | |||
|
|
|||
| def acceptable?(val) | |||
| unless val.instance_of?(type_class) || val.respond_to?(:to_hash) | |||
| unless val.is_a?(type_class) || val.is_a?(Hash) | |||
There was a problem hiding this comment.
this may be a bit outside the scope of this fix. The reason I made this change was because this line was failing without it. IT seemed wrong that an incorrect object that has no relationship to the expected object should be acceptable. It was passing this method because of val.respond_to?(:to_hash), which seems too open ended. Also, using instance_of? doesn't take into account class inheritance while is_a? does.
There was a problem hiding this comment.
I can see the change for is_a? but the swap from respond_to seems unnecessary. We use duck type checks all over in this lib purposefully. You should be able to pass a hash-like object here. If you change that back I'll get this merged.
There was a problem hiding this comment.
hmmm, if I move this to unless val.is_a?(type_class) || val.respond_to?(:to_hash), then this test fails.
I put that test in there because assigning an unrelated object to a MessageField and having it pass (and possibly drop fields silently if they don't match) seems wrong. But if that is ok or the desired behavior I'll go ahead and make the change and remove the test.
There was a problem hiding this comment.
The only reason I think that's ok is because you could do OtherBasicMessage.new.to_hash and pass it down... the message will ignore any fields it doesn't know about so it seems ok to me. Thoughts?
|
just a polite ping :) Let me know if you want or need additional tests or documentation. thanks! |
| @@ -66,6 +66,8 @@ def normalize(value) | |||
|
|
|||
| if field.is_a?(::Protobuf::Field::EnumField) | |||
| field.type_class.fetch(value) | |||
| elsif field.is_a?(::Protobuf::Field::MessageField) && value.is_a?(field.type_class) | |||
There was a problem hiding this comment.
Ahh, makes sense. Good spot.
|
Sorry for the radio silence, if you can revert that respond_to change I'll get this merged. |
|
@localshred no worries! I understand you have a life outside of your OS projects :) |
don't downcast when assigning to a field_array
|
@localshred and my apologies for going silent! If the changes to |
No description provided.