marcoferrer / kroto-plus

gRPC Kotlin Coroutines, Protobuf DSL, Scripting for Protoc
Apache License 2.0
493 stars 28 forks source link

Support for `google/protobuf/wrappers.proto` #15

Closed rocketraman closed 5 years ago

rocketraman commented 5 years ago

The Google protobuf standard wrappers are useful with proto3, to handle nullable fields. It would be nice if kroto-plus added explicit support for these wrappers e.g. generate builders for them if they are referenced in the proto files.

rocketraman commented 5 years ago

Definitions like this could conceivably just be placed into a runtime library:

object ProtobufWrappers {
  fun stringValue(value: String): StringValue {
    return StringValue.newBuilder().apply {
      this.value = value
    }.build()
  }
}

The caller has to do things like this, to only set the value if it is not null -- not sure if there is a way to enhance builders that use these wrappers to avoid the extra rigmarole:

// Foo is a Krotoplus generated builder, that contains `myField` of type `StringValue`
Foo {
  // sourceOfMyField is String?
  sourceOfMyField?.let { myField = stringValue(it) }
}
marcoferrer commented 5 years ago

So the latest kroto version should be generating builder extensions for nested types. Which means this syntax should be valid.

Foo{
    sourceOfMyField?.let { 
        myField { value = it } 
    }
}

But even this usage isnt really ideal.

I do know that out of the box most of the wrappers have utility methods defined to simplify building. eg.

Foo{
    sourceOfMyField?.let { 
        myField = StringValue.of(it)
    }
}

If you still want to generate builders or utility exts such as copy, plus, and soon orDefault, then you should be able to add the following to your config and get the desired output.

proto_builders {
    filter {
        include_path: "google/protobuf/wrappers.proto"
    }
    unwrap_builders: true
}

Are these options sufficient for your use case?

I do think there might be a better way to do this in the future.

rocketraman commented 5 years ago

Are these options sufficient for your use case?

Yes, for now I'll go with your second option -- I didn't see that utility method. The first option does work, but IMO is a little too opaque readability-wise.

Thanks!

Y0ba commented 5 years ago

Could you make them just nullable types? For example in C# StringValue is just a plain string value which can accept null values and do all conversions to StringValue under the hood. In kotlin StringValue can be just String? type by default set to null.

rocketraman commented 5 years ago

That would be amazing. However, and @marcoferrer can correct me if I'm wrong, I think that requires deeper integration than krotoplus does today, which is to basically wrap the upstream java builders.

rocketraman commented 5 years ago

As a side note, the opposite of accessing wrapper data from Kotlin is annoying too, because you have to call hasX from the parent Object, you can't just create some nice reusable extensions on the wrappers.