ruby-protobuf / protobuf

A pure ruby implementation of Google's Protocol Buffers
https://github.com/ruby-protobuf
MIT License
463 stars 101 forks source link

Track if a repeated field has been deliberately set #325

Closed embark closed 8 years ago

embark commented 8 years ago

Before, if an unset repeated field was ever read, we would make an empty field array for it. As a consequence, .field? and #{field}! methods would indicate that the field had been set, even though it had only been read from. This was a weird side effect of reading an unset repeated field and has been corrected.

CC @film42 @nerdrew

Next piece of ruby-protobuf#323

nerdrew commented 8 years ago

LGTM

embark commented 8 years ago

@film42 Hoping for a review on this, then will fast-follow with another PR

film42 commented 8 years ago

Nice bug catch! This is a good fit for 3.7.0 because it subtly changes behavior (though this behavior has been a bug).

I think this is ready to go, but I'm going to ping @liveh2o on this to see what he thinks about this change. I usually ask him for advice about planning breaking changes.

Concise example of the bug:

m = ::Test::Resource.new(:name => "thing"); nil
m.repeated_enum! #=> nil -- expected
m.repeated_enum #=> [] -- expected
m.repeated_enum! #=> [] -- this is should be nil

Current behavior also considers repeating fields initialized with [] to be nil (3.6.X):

m = ::Test::Resource.new(:name => "thing", :repeated_enum => []); nil
m.repeated_enum! #=> nil
liveh2o commented 8 years ago

This is subtle, but ultimately matches the current behavior when initializing an empty repeated enum. I think we're OK moving this forward.

:shipit:

embark commented 8 years ago

@film42 thanks for the review! is this good to merge?

film42 commented 8 years ago

Yes! I'll cut a new pre tomorrow morning. Thanks for the reminders, @embark!

film42 commented 8 years ago

3.7.0.pre2 has been released. Thank you!