Group decoder support (inefficient)

[nviennot]

No description provided.

[localshred]

Can you provide an example proto that would illustrate a group? I'd like to test through this.

[nviennot]

Sure: https://github.com/egirault/googleplay-api/blob/master/googleplay.proto

[localshred]

Your code only adds support for decoding an encoded group, does that mean you are expecting support for being able to encode groups as well? I'd rather not merge this in until we had full compiler, encode, and decode support.

[nviennot]

I don't need to serialize groups, so I probably won't do it.

The compiler (proto to ruby by using rproto if I'm understanding correctly) supports groups well.

[localshred]

Sure, but I'm saying it doesn't make sense for this library to only support decoding of groups, we should add encoding as well before supporting either side.

I wrote the rprotoc compiler as well and we definitely don't support groups at this point (though I'm not opposed to it of course).

[nviennot]

well, I'm using the rprotoc compiler you created, and it works very well with groups.

Having said that, I understand that without the serialization routines, this patch is funny.

[localshred]

Fascinating the compiler is working with it. Can you post the proto and compiled ruby for a group? I'd like to see how the descriptor API is providing the groups. It may be simply that we add a DSL method to the class to indicate that the type is a Group. This would allow us to encode with the group header for that type.

I don't think you should close, but perhaps we can work together to add the encoding step.

[nviennot]

Example of a protobuf with groups:

message PurchaseInfoProto {
  optional string transactionId = 1;
  optional PurchaseCartInfoProto cartInfo = 2;
  optional group BillingInstruments = 3 {
    repeated group BillingInstrument = 4 {
      optional string id = 5;
      optional string name = 6;
      optional bool isInvalid = 7;
      optional int32 instrumentType = 11;
      optional int32 instrumentStatus = 14;
    }
    optional string defaultBillingInstrumentId = 8;
  }
  repeated int32 errorInputFields = 9;
  optional string refundPolicy = 10;
  optional bool userCanAddGdd = 12;
  repeated int32 eligibleInstrumentTypes = 13;
  optional string orderId = 15;
}

Generated ruby:

  class PurchaseInfoProto
    class BillingInstruments
      class BillingInstrument
        optional ::Protobuf::Field::StringField, :id, 5
        optional ::Protobuf::Field::StringField, :name, 6
        optional ::Protobuf::Field::BoolField, :is_invalid, 7
        optional ::Protobuf::Field::Int32Field, :instrument_type, 11
        optional ::Protobuf::Field::Int32Field, :instrument_status, 14
      end

      repeated ::GooglePlay::PurchaseInfoProto::BillingInstruments::BillingInstrument, :billinginstrument, 4
      optional ::Protobuf::Field::StringField, :default_billing_instrument_id, 8
    end

    optional ::Protobuf::Field::StringField, :transaction_id, 1
    optional ::GooglePlay::PurchaseCartInfoProto, :cart_info, 2
    optional ::GooglePlay::PurchaseInfoProto::BillingInstruments, :billinginstruments, 3
    repeated ::Protobuf::Field::Int32Field, :error_input_fields, 9
    optional ::Protobuf::Field::StringField, :refund_policy, 10
    optional ::Protobuf::Field::BoolField, :user_can_add_gdd, 12
    repeated ::Protobuf::Field::Int32Field, :eligible_instrument_types, 13
    optional ::Protobuf::Field::StringField, :order_id, 15
  end

---

If you never intended to support them, congrats!!!! It means that the abstractions you chose are really working well :)

[localshred]

Hahaha, fortunately those abstractoins are simply the FieldDescriptor and Descriptor objects that the parser gives me in the compiler. :) Woot for open source. So yes, I think we should simply devise a group DSL such that we could identify the repeated billinginstrument in your example as a grouped field for the encoder to determine.

[nviennot]

Okay, even though it's probably trivial to do the serialization routine, I'm not going to do it at the moment, I have an android market to crawl, and I'm under tight deadlines. I'll come back to you if I find the time to do it :)

Thanks for the gem btw 👍

[localshred]

Oh you are very welcome. I'll take a stab at the encoding if I get a spare hour in the next few days. Thanks again for the contributions.

[localshred]

Found where I added compiler support for group fields. Honestly don't remember adding it, but there it is.

The only thing to do then is write an option on the end of the field definition :group => true. Then at encode time check the field options and if it's a group field, write the group header.

👍

[localshred]

I'm going to close this for now. We may add this in the future but I'm not ready to commit to it (as evidence that the PR has been open for 10 months).