Field Presence for Protocol Buffer C# #321
Conversation
Csharp update
|
@jskeet |
| BAZ = 2; | ||
| } | ||
| message NestedMessage { | ||
| optional int32 value = 1; |
There was a problem hiding this comment.
Given that this is proto3, I'd remove the "optional" everywhere. (It's at least non-idiomatic.)
|
Nits, mostly - and I can fix up the runtime code if you'd like me to. |
|
Btw, I'm happy to fix up these new tests in conjunction with pull request 324 - I don't mind which gets merged first. |
|
Have merged PR #324 in - you could either fix the tests to use xUnit, or I can do so in a separate PR. (I don't think being broken very temporarily is a significant issue at this point.) |
|
Thank you Jon Skeet. Have fixed most comments. |
| @@ -42,6 +43,7 @@ internal class SinglePrimitiveAccessor<TMessage, TBuilder> : IFieldAccessor<TMes | |||
| where TBuilder : IBuilder<TMessage, TBuilder> | |||
| { | |||
| private readonly Type clrType; | |||
| private readonly FieldDescriptor field; | |||
There was a problem hiding this comment.
Nit: we don't need this field any more (as we're just capturing the descriptor in the delegate where we need it).
| } | ||
|
|
||
| [Fact] | ||
| public void TestSeralizeAndParse() |
There was a problem hiding this comment.
Sorry - missed the typo here before: Seralize => Serialize.
|
Added a few more small notes - but I'd be happy to fix them up myself if that's easier. Is @jtattermusch reviewing the C++? (Basically LGTM on the C#.) |
|
The C++ changes generally LGTM. Good to go after addressing Jon's and my comments. |
Field Presence for Protocol Buffer C# Proto3
|
Have fixed comments and merged. Thanks for reviewing |
…uffers#325) Closes protocolbuffers#321. In "nested_maps/2", the fully qualified name was built without discarding the nil entries of the list. This meant that map fields worked correctly for .proto files that contained a package, but failed to do so in .proto files that did not, since the map key had then an extra "." in front that made the lookup in "get_field/4" fail.
Remove has methods for optional non-message fields in proto3