Protobuf dynamic typing in C# #11092
Conversation
himkak
commented
Nov 30, 2022
himkak
commented
- Added support for Dynamic typing by adding DynamicMessage for C#. The implementation closely follows the implementation done in java.
- Added dotnet workflow for build and test C# code
|
I'll have a look at this when I get some time, but it may not be for a while I'm afraid. |
|
Sure, thanks. |
I strongly suspect we won't be in a position to merge until some time in 2023. Protobuf isn't my main role, so I'll only be able to put time into reviewing periodically. Do you have an urgent need for this for some reason? |
Yes, I am using these changes in my project, and looking to use the released version of the library. |
No, we really don't have enough C#-suitable resources - and close to the end of the year, lots of people are taking vacation time etc. |
There was a problem hiding this comment.
Overall, this isn't going in a direction I'd want to merge. I'd expect DynamicMessage to support:
- Being handled like any other IMessage
- JSON parsing and formatting
- Reflection access via
message.Descriptor.FindField(...).Accessor.GetValue(message)etc - Unknown fields
This all requires quite a bit of work, which is why it hasn't been done yet.
Given that this PR doesn't need to modify any existing code, I suggest that if it satisfies your needs, you keep it within your own codebase for now, and then migrate to whatever full support we end up with eventually.
.github/workflows/dotnet.yml
Outdated
| @@ -0,0 +1,26 @@ | |||
| # This workflow will build a .NET project | |||
There was a problem hiding this comment.
Please don't add a workflow as part of an entirely separate feature.
We already have testing via Kokoro.
| using System; | ||
| using System.Collections; | ||
|
|
||
| namespace Google.Protobuf.Reflection.Dynamic |
There was a problem hiding this comment.
I think we'll be able to use just Google.Protobuf.Reflection for this. I don't think a new sub-namespace is required.
|
|
||
| /// <summary> | ||
| /// Default implementation of Overridden method. | ||
| /// This specific implementation is never used, mergeFrom is used from the builder class. |
There was a problem hiding this comment.
This prohibits it from being used in code that just uses IMessage.
| /// </summary> | ||
| /// <param name="type"></param> | ||
| /// <returns></returns> | ||
| public static Builder NewBuilder(MessageDescriptor type) |
There was a problem hiding this comment.
The Java origins of this PR are really showing here. We don't use builders in the .NET implementation; instead, all messages are mutable. I'd expect DynamicMessage to follow that pattern.
|
Thanks a lot for quickly reviewing this PR. |
|
You're welcome to try - but having looked at this a few times (and having worked in this codebase for 14 years) it's always felt like a daunting prospect to me, with changes required across the reflection code as well as creating the core |
|
I have created a new PR in my forked repo itself. It tries to follow the same pattern as other IMessage implementations do. |
|
I'm on vacation now until January. I'll have a look next time I'm working. |
|
Sure, Merry Christmas and Happy New Year in advance. Enjoy your vacation. |
