Skip to content

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

Merged
merged 1 commit into from
Mar 18, 2016

Conversation

embark
Copy link
Contributor

@embark embark commented Mar 1, 2016

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


class ToProtoIsForWrongMessage
def to_proto
SomeMessage.new()
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, thanks! FIxed

@zachmargolis
Copy link
Contributor

LGTM! thanks for fixing

@nerdrew
Copy link
Contributor

nerdrew commented Mar 2, 2016

+1

@embark
Copy link
Contributor Author

embark commented Mar 4, 2016

@liveh2o @film42 Sorry for the ping, but does this look okay? Don't like to leave bugs around.

@@ -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)
Copy link
Contributor

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:

  1. Ignore nil values
  2. Try and coerce the value
  3. Fail unless the coerced value is the correct type

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@liveh2o I think this patch is pretty reasonable as-is, I'm happy leaving it as @embark proposed?

Copy link
Contributor Author

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.

@liveh2o
Copy link
Contributor

liveh2o commented Mar 5, 2016

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.
@embark
Copy link
Contributor Author

embark commented Mar 8, 2016

Comments addressed :)

@liveh2o
Copy link
Contributor

liveh2o commented Mar 9, 2016

:shipit:

@liveh2o
Copy link
Contributor

liveh2o commented Mar 9, 2016

It looks like we've got a few PRs stacking up. I'll check with @film42 and see we can cut a release tomorrow.

film42 added a commit that referenced this pull request Mar 18, 2016
Messages fields should be able to be set with to_proto
@film42 film42 merged commit 88df1e7 into ruby-protobuf:master Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants