-
Notifications
You must be signed in to change notification settings - Fork 100
Messages fields should be able to be set with to_proto #309
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
Conversation
|
||
class ToProtoIsForWrongMessage | ||
def to_proto | ||
SomeMessage.new() |
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.
style nit, drop the ()
when there are no arguments
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.
fixed by running rubocop :)
require 'spec_helper' | ||
|
||
RSpec.describe Protobuf::Field::MessageField do | ||
class MessageField < ::Protobuf::Message |
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.
just as a heads up, this will leak the name MessageField
into the global namespace in all specs, see Class.new
in things like enum_spec.rb
and message_spec.rb
for how to define anonymous classes in tests that don't leak?
it's not the end of the world but a good practice IMO in case some other test tries to make a class named MessageField
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.
Ah right, thanks! FIxed
LGTM! thanks for fixing |
+1 |
@@ -35,8 +35,8 @@ def coerce!(value) | |||
nil | |||
elsif value.is_a?(type_class) | |||
value | |||
elsif value.respond_to?(:to_proto) | |||
value.to_proto | |||
elsif value.respond_to?(:to_proto) && (proto_value = value.to_proto).is_a?(type_class) |
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.
Is this check necessary? If so, the complexity of a compound conditional that also sets a variable that is used as the return value is something we should try to eliminate. If the goal is to avoid having to write the failure line twice, then I'd rewrite the method:
def coerce!(value)
return nil if value.nil?
coerced_value = if value.respond_to?(:to_proto)
value.to_proto
elsif value.respond_to?(:to_hash)
type_class.new(value.to_hash)
else
value
end
return coerced_value if coerced_value.is_a?(type_class)
fail TypeError, "Expected value of type '#{type_class}' for field #{name}, but got '#{value.class}'"
end
To me, this is much clearer. It basically has three steps:
- Ignore nil values
- Try and coerce the value
- Fail unless the coerced value is the correct type
What do you think?
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.
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.
I'm actually good with the proposal, it might be more readable and sounds like it's faster? I changed it as suggested.
Don't hesitate to ping if you're not getting a response. We need to stay on top of this stuff. |
A prior commit introduced a bug so that you would get `Failure/Error: fail TypeError, "Unacceptable value #{value} for field #{field.name} of type #{field.type_class}"` when trying to set a message field with a class that defined a #to_proto method.
Comments addressed :) |
|
It looks like we've got a few PRs stacking up. I'll check with @film42 and see we can cut a release tomorrow. |
Messages fields should be able to be set with to_proto
A prior commit (#302) introduced a bug so that you would get
Failure/Error: fail TypeError, "Unacceptable value #{value} for field #{field.name} of type #{field.type_class}"
when trying to set a message field with a class that defined a #to_proto method.CC @zachmargolis @nerdrew @film42