search

protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.11k stars 15.43k forks source link

Add FieldMaskUtil support in C# #4325

Closed Falco20019 closed 6 years ago

Falco20019 commented 6 years ago

As of today, the C# implementation is missing support for working with FieldMask. The message itself exists in Google.Protobuf.WellKnownTypes, so it can be used for merging in other languages. But since the FieldMaskUtil is missing in C#, merging and working with FieldMasks in general has to be done manually.

If no one is working on it, I could also tackle it since I need it myself. Just wanted to make sure it's not done by anyone else right now or if there is a cause why it's not implemented.

anandolee commented 6 years ago

Runtime support for FieldMask is optional. There's no specification for FieldMask support. C# has a very limited FieldMask support in FieldMaskPartial.cs.

Merge is not implemented in FieldMaskPartial.cs. Are you happy to create a PR add merge? You can take the c++ version as an example: https://github.com/google/protobuf/blob/master/src/google/protobuf/util/field_mask_util.cc#L208

Falco20019 commented 6 years ago

I will create an PR for it when I have some time the next 1-2 months :) Right now I have some other construction sites like tracing to finalize. But this should be fine since not too many people seem to need this feature.

anandolee commented 6 years ago

@Falco20019, I am closing this issue for clean up. Once the PR is ready to review, feel free to assign to me for review.

Falco20019 commented 6 years ago

@anandolee Do you prefer to have it in FieldMaskPartial or should I add a Util namespace and keep the FieldMaskUtil and FieldMaskTree in it's separate classes? Last one would be the cleaner option in my opinion, but would be a break with the helpers of other Partial classes like with Duration.