C# Proto2 feature : Extensions #5350
Conversation
src/google/protobuf/compiler/csharp/csharp_source_generator_base.cc
Outdated
Show resolved
Hide resolved
|
Can you also make sure this PR is work with C# 6.0? We've got an issue for C# 6.0 compatible, and was working on it in #5445 |
|
Sure but after #5445 is pulled so i can rebase on top of it. |
ObsidianMinor
force-pushed
the
csharp/proto2-feature/extensions
branch
from
9084c23
to
2ee1a20
Compare
|
Now C# 6 compatible. Can we get another kokoro run, @anandolee? |
ObsidianMinor
force-pushed
the
csharp/proto2-feature/extensions
branch
from
4641f40
to
8fe536a
Compare
There was a problem hiding this comment.
@jskeet , are you able to help review this PR? This change added extension and will Obsolete custom options. I'd like to make sure the basic APIs looks good to you before we enable proto2 generator as you are the major contributor.
Note: tests are not added because we haven't enable proto2 generator yet. All proto2 support PRs (including #4642 #5183 and this one) did not affect users yet and we could make API change. Related tests will be added in the final proto2 support PR which enables proto2 generator.
|
I'm afraid I wouldn't have time to look at this until mid January at the earliest |
There was a problem hiding this comment.
API changes will happen next PR, @anandolee, as they require public descriptor protos.
|
@jskeet |
|
I'll see when the time comes, but hopefully I'll be able to help. It'll have to be somewhat time limited though. |
|
This seems has been opened for quite some time. Any updates? |
|
I believe we're still waiting for @anandolee to review @lostindark |
…essage interface.
ObsidianMinor
force-pushed
the
csharp/proto2-feature/extensions
branch
from
2e6d38c
to
b72a3ff
Compare
|
I think you also need to add a "release notes: no" label to pass Mergable's checks @anandolee |
There was a problem hiding this comment.
A few additional nits that would be good to address in #5936
| { | ||
| return new FieldCodec<T>(input => { T message = parser.CreateTemplate(); input.ReadMessage(message); return message; }, | ||
| (output, value) => output.WriteMessage(value), message => CodedOutputStream.ComputeMessageSize(message), tag); | ||
| (output, value) => output.WriteMessage(value), (CodedInputStream i, ref T v) => |
There was a problem hiding this comment.
I feel like this method is not very readable (also below)
There was a problem hiding this comment.
Some more comments.
The changes have only been released as part of 3.9.0, which is currently unlisted on nuget, so I think the comments should be addressed in v3.9.1 if the extensions API is exposed in the public API in any useful way (or is the API completely useless right now and it's going to be only start being usable with #5936?)
I'm still reviewing the Extension, ExtensionSet, ExtensionValue, ... APIs to make sure they are making sense. What is the best place to look at to see the intended usage of extensions in C#?
CC @anandolee
Correct, you can't generate any code that uses Extension or ExtensionSet with this PR.
There's many tests you can browse to see how to use extensions (ExtensionSet and ExtensionValue are not exposed).
|
| IExtensionValue value; | ||
| if (first.ValuesByNumber.TryGetValue(pair.Key, out value)) | ||
| { | ||
| value.MergeFrom(pair.Value); |
There was a problem hiding this comment.
if the extension value happens to be of the wrong kind (ExtensionValue vs RepeatedExtensionValue), extensionValue.MergeFrom(stream) silently ignores the value (there's a "is" check). Is that intended?
There was a problem hiding this comment.
Yes, is that not the correct behavior for merging?
There was a problem hiding this comment.
not sure. @anandolee would know better?
We're at the final stretch.
This PR adds support for extensions in the C# library and code generator.