Clean up dynamic code generation in prep for extension namespacing #302

embark · 2016-02-12T01:06:57Z

zachmargolis · 2016-02-12T02:18:56Z

lib/protobuf/message.rb

+        if field.repeated?
+          if value.is_a?(Array)
+            value = value.dup
+            value.compact!


I know this is preserving existing behavior, but just want to note that IMO this compact thing is super weird behavior, settings an array with nil values in there should def be an error

If you're going to keep compact!, you should remove the dup and just do value = value.compact. You can drop an array iteration that way and get the same behavior.

Good call. Note though: We copied the code over with minimal changes. No objection to cleaning as we go though.

zachmargolis · 2016-02-12T02:28:37Z

Should we add a test that verified that the fully qualified names are properly used as keys in @values to make sure they are set correctly?

zanker · 2016-02-12T03:46:09Z

lib/protobuf/field/base_field.rb

-                           when optional? then typed_default_value
-                           when repeated? then ::Protobuf::Field::FieldArray.new(self).freeze
-                           when required? then nil
+        @default_value ||= if optional? || required?


Isn't adding required? unnecessary because required? should always have a value.

lol, ya, but protoc allows default values on required fields, sooooo... we should handle it?

embark · 2016-02-12T21:01:58Z

re: #302 (comment)

We don't yet actually use fully qualified names, but there's a PR incoming where we will add a test for it

zachmargolis · 2016-02-12T21:44:14Z

lib/protobuf/field/base_field.rb

+                           elsif repeated?
+                             ::Protobuf::Field::FieldArray.new(self).freeze
+                           else
+                             fail "something went very wrong"


maybe something like "unable to produce default value" or something? very ominous message

Well this code should be unreachable actually, so it should be ominous -- it means a field isn't optional, required, or repeated. We could make change the message though to still be ominous while more descriptive :)

embark · 2016-02-16T17:33:46Z

All comments are addressed, interested in review by someone with write access to the repo :)

embark · 2016-02-23T17:55:28Z

Hey all, I'm looking for reviews from some people with write access because there will be a few commits following up to this with the goal of adding custom option support to the gem. Pinging a couple people who have merged recently (many apologies for the noise).

CC @film42 @liveh2o

liveh2o · 2016-02-23T18:07:56Z

lib/protobuf/field/base_field.rb

@@ -122,10 +124,10 @@ def required?

      # FIXME: need to cleanup (rename) this warthog of a method.
      def set(message_instance, bytes)
-        return message_instance.__send__(setter, decode(bytes)) unless repeated?
-        return message_instance.__send__(getter) << decode(bytes) unless packed?
+        return message_instance[name] = decode(bytes) unless repeated?


Does this still flow through the setter? I'm exactly sure why we needed to be calling the setter here, but I want to make sure this isn't unintentionally altering behavior.

Though perhaps that is the point here?

The []= method now is the setter and [] is now the getter, so it should be the same. The old setter and getter still exist for runtime access of the proto, but they alias to the former two methods now.

For just a little more background, [] and []= used to alias to the setter and getter instead, but we switched that around so that we didn't have this weird dynamic usage of the setters and getters.

Got it. No more __send__(xxx).

brianstien · 2016-02-23T19:06:42Z

@embark I don't have write access, but for any reviewers that do, I reviewed this and 👍.

Flipping around the [], []= with the setters makes the internals easier to read. I also like using the coerce! interface instead of overriding the define_setter method.

film42 · 2016-02-23T20:24:52Z

Just remove the @encode and I'll cut a release. Thank you for your contribution! :)

embark · 2016-02-23T21:31:31Z

Many thanks to all the reviews and responses! -- Removed @encode so all should be well

embark · 2016-02-23T22:35:10Z

CC @film42 all green!

film42 · 2016-02-24T05:34:23Z

Clean up dynamic code generation in prep for extension namespacing

film42 · 2016-02-24T05:41:51Z

Hmm. I used to have push access to rubygems.. looks like someone is going to need to add me again. Tomorrow I'll cut this as 3.7.0.pre0.

embark · 2016-02-24T06:38:11Z

@film42 Thanks! There will be a few PRs to come to get support for custom options if you want to wait for that before cutting a new release, though it's up to you.

embark · 2016-03-02T18:31:42Z

FYI @film42 @liveh2o @zachmargolis @nerdrew I just discovered that this PR changed the behavior of the []= setter to be correct in the case when nil is passed to it as a value.

Old behavior for repeated fields:

% pry(main)> message = ::Test::TestMessage.new(repeated: [1,2])
=> #<Test::TestMessage repeated =[1, 2]>

% pry(main)> message['repeated'] = nil
=> nil

% pry(main)> message
=> #<Test::TestMessage repeated =[1, 2]>

note that nothing happens when nil is passed in.

New behavior for repeated fields:

% pry(main)> message['repeated'] = ::Test::TestMessage.new(repeated: [1,2])
=> #<Test::TestMessage repeated =[1, 2]>

% pry(main)> message['repeated'] = nil
TypeError:                 Expected repeated value of type 'Protobuf::Field::Int32Field'
                Got 'NilClass' for repeated protobuf field rep

% pry(main)> message
=> #<Test::TestMessage rep=[1, 2]>

Old behavior for optional fields:

% pry(main)> message = ::Test::TestMessage.new(optional: 1)
=> #<Test::TestMessage optional = 1>

% pry(main)> message['optional'] = nil
=> nil

% pry(main)> message
=> #<Test::TestMessage optional = 1>

again, nothing happens to the field, it isn't even cleared.

New behavior for optional fields:

% pry(main)> message = ::Test::TestMessage.new(optional: 1)
=> #<Test::TestMessage optional = 1>

% pry(main)> message['optional'] = nil
=> nil

% pry(main)> message
=> #<Test::TestMessage optional = 0>

now the optional field has been cleared.

NOTE: the new []= behavior now matches the behavior of the dynamically defined setters (e.g. message.repeated and message.optional). It was a bug to silently do nothing in the []= setter when nil was passed in.

…ters Clean up dynamic code generation in prep for extension namespacing

embark force-pushed the fix-setters-and-getters branch 2 times, most recently from 7d8247e to 133207b Compare February 12, 2016 01:51

zachmargolis reviewed Feb 12, 2016
View reviewed changes

zanker reviewed Feb 12, 2016
View reviewed changes

embark force-pushed the fix-setters-and-getters branch from 133207b to b0f62ba Compare February 12, 2016 21:38

zachmargolis reviewed Feb 12, 2016
View reviewed changes

embark force-pushed the fix-setters-and-getters branch 2 times, most recently from 065400e to 49412d1 Compare February 15, 2016 21:56

embark force-pushed the fix-setters-and-getters branch from 49412d1 to 3176340 Compare February 17, 2016 19:04

nerdrew mentioned this pull request Feb 17, 2016

Namespace extension fields #303

Merged

embark force-pushed the fix-setters-and-getters branch from 3176340 to 10c3ea1 Compare February 17, 2016 19:06

embark closed this Feb 22, 2016

embark reopened this Feb 22, 2016

nerdrew force-pushed the fix-setters-and-getters branch from 10c3ea1 to 4b85f47 Compare February 23, 2016 17:31

embark force-pushed the fix-setters-and-getters branch from 4b85f47 to 10c3ea1 Compare February 23, 2016 17:34

nerdrew force-pushed the fix-setters-and-getters branch from 10c3ea1 to 1ceee2f Compare February 23, 2016 17:37

liveh2o reviewed Feb 23, 2016
View reviewed changes

embark force-pushed the fix-setters-and-getters branch from 1ceee2f to ad05d79 Compare February 23, 2016 20:28

nerdrew force-pushed the fix-setters-and-getters branch from ad05d79 to 1ceee2f Compare February 23, 2016 20:30

Clean up dynamic code generation in prep for extension namespacing

154fbad

embark force-pushed the fix-setters-and-getters branch from 1ceee2f to 154fbad Compare February 23, 2016 20:32

embark mentioned this pull request Feb 23, 2016

Abrandoned/decoding setters without copy #297

Open

film42 added a commit that referenced this pull request Feb 24, 2016

Merge pull request #302 from nerdrew/fix-setters-and-getters

757068b

Clean up dynamic code generation in prep for extension namespacing

film42 merged commit 757068b into ruby-protobuf:master Feb 24, 2016

embark deleted the fix-setters-and-getters branch February 24, 2016 06:52

embark mentioned this pull request Mar 1, 2016

Messages fields should be able to be set with to_proto #309

Merged

embark pushed a commit to nerdrew/protobuf-gem that referenced this pull request Mar 2, 2016

Merge pull request ruby-protobuf#302 from nerdrew/fix-setters-and-get…

5f6fb2d

…ters Clean up dynamic code generation in prep for extension namespacing

embark mentioned this pull request Mar 2, 2016

Empty values should not clear a protobuf field #310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up dynamic code generation in prep for extension namespacing #302

Clean up dynamic code generation in prep for extension namespacing #302

embark commented Feb 12, 2016

zachmargolis Feb 12, 2016

zanker Feb 12, 2016

nerdrew Feb 12, 2016

zachmargolis commented Feb 12, 2016

zanker Feb 12, 2016

nerdrew Feb 12, 2016

zanker Feb 12, 2016

embark commented Feb 12, 2016

zachmargolis Feb 12, 2016

embark Feb 12, 2016

embark commented Feb 16, 2016

embark commented Feb 23, 2016

liveh2o Feb 23, 2016

liveh2o Feb 23, 2016

embark Feb 23, 2016

liveh2o Feb 23, 2016

brianstien commented Feb 23, 2016

film42 commented Feb 23, 2016

embark commented Feb 23, 2016

embark commented Feb 23, 2016

film42 commented Feb 24, 2016

film42 commented Feb 24, 2016

embark commented Feb 24, 2016

embark commented Mar 2, 2016

Clean up dynamic code generation in prep for extension namespacing #302

Clean up dynamic code generation in prep for extension namespacing #302

Conversation

embark commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmargolis commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embark commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embark commented Feb 16, 2016

embark commented Feb 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianstien commented Feb 23, 2016

film42 commented Feb 23, 2016

embark commented Feb 23, 2016

embark commented Feb 23, 2016

film42 commented Feb 24, 2016

film42 commented Feb 24, 2016

embark commented Feb 24, 2016

embark commented Mar 2, 2016