Skip to content

C# Proto2 feature : Extensions#5350

Merged
anandolee merged 27 commits intoprotocolbuffers:masterfrom
ObsidianMinor:csharp/proto2-feature/extensions
May 3, 2019
Merged

C# Proto2 feature : Extensions#5350
anandolee merged 27 commits intoprotocolbuffers:masterfrom
ObsidianMinor:csharp/proto2-feature/extensions

Conversation

@ObsidianMinor
Copy link
Copy Markdown
Contributor

@ObsidianMinor ObsidianMinor commented Nov 14, 2018

We're at the final stretch.

This PR adds support for extensions in the C# library and code generator.

@TeBoring TeBoring requested a review from anandolee November 19, 2018 21:38
@TeBoring TeBoring added the c# label Nov 19, 2018
anandolee
anandolee reviewed Nov 29, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@anandolee anandolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice we are close to proto2!

anandolee
anandolee reviewed Dec 6, 2018
View reviewed changes
@anandolee
Copy link
Copy Markdown
Contributor

anandolee commented Dec 10, 2018

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

@ObsidianMinor
Copy link
Copy Markdown
Contributor Author

ObsidianMinor commented Dec 11, 2018

Sure but after #5445 is pulled so i can rebase on top of it.

@ObsidianMinor ObsidianMinor force-pushed the csharp/proto2-feature/extensions branch from 9084c23 to 2ee1a20 Compare December 12, 2018 02:02
@ObsidianMinor
Copy link
Copy Markdown
Contributor Author

ObsidianMinor commented Dec 12, 2018

Now C# 6 compatible. Can we get another kokoro run, @anandolee?

@ObsidianMinor ObsidianMinor force-pushed the csharp/proto2-feature/extensions branch from 4641f40 to 8fe536a Compare December 15, 2018 00:42
@anandolee anandolee requested a review from jskeet December 17, 2018 22:15
anandolee
anandolee reviewed Dec 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@anandolee anandolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@jskeet
Copy link
Copy Markdown
Contributor

jskeet commented Dec 17, 2018

I'm afraid I wouldn't have time to look at this until mid January at the earliest

ObsidianMinor
ObsidianMinor commented Dec 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@ObsidianMinor ObsidianMinor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API changes will happen next PR, @anandolee, as they require public descriptor protos.

@anandolee
Copy link
Copy Markdown
Contributor

anandolee commented Dec 18, 2018 •
edited
Loading

@jskeet
Understand. Most people will be OOO for next two weeks. I will review this PR. Can you help review the next (final) PR? The next PR will enable proto2 generator, change the descriptor protos public, enable all the APIs Sydney has added and will add all the tests. I assume it will be late January or February

@jskeet
Copy link
Copy Markdown
Contributor

jskeet commented Dec 18, 2018

I'll see when the time comes, but hopefully I'll be able to help. It'll have to be somewhat time limited though.

@lostindark
Copy link
Copy Markdown

lostindark commented Feb 6, 2019

This seems has been opened for quite some time. Any updates?
I'm eager to test proto2 support.

@ObsidianMinor
Copy link
Copy Markdown
Contributor Author

ObsidianMinor commented Feb 6, 2019

I believe we're still waiting for @anandolee to review @lostindark

anandolee
anandolee reviewed Feb 21, 2019
View reviewed changes
@ObsidianMinor ObsidianMinor force-pushed the csharp/proto2-feature/extensions branch from 2e6d38c to b72a3ff Compare May 2, 2019 18:50
@ObsidianMinor
Copy link
Copy Markdown
Contributor Author

ObsidianMinor commented May 2, 2019

I think you also need to add a "release notes: no" label to pass Mergable's checks @anandolee

@anandolee anandolee merged commit 9e89b6e into protocolbuffers:master May 3, 2019
jtattermusch
jtattermusch reviewed Jul 17, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few additional nits that would be good to address in #5936

csharp/src/Google.Protobuf/FieldCodec.cs
{
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) =>
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this method is not very readable (also below)

@jtattermusch jtattermusch mentioned this pull request Jul 17, 2019
Closed
jtattermusch
jtattermusch reviewed Jul 19, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jtattermusch
jtattermusch reviewed Jul 19, 2019
View reviewed changes
jtattermusch
jtattermusch reviewed Jul 19, 2019
View reviewed changes
@ObsidianMinor
Copy link
Copy Markdown
Contributor Author

ObsidianMinor commented Jul 19, 2019

(or is the API completely useless right now and it's going to be only start being usable with #5936?)

Correct, you can't generate any code that uses Extension or ExtensionSet with this PR.

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#?

There's many tests you can browse to see how to use extensions (ExtensionSet and ExtensionValue are not exposed).

jtattermusch
jtattermusch reviewed Jul 19, 2019
View reviewed changes
jtattermusch
jtattermusch reviewed Jul 19, 2019
View reviewed changes
jtattermusch
jtattermusch reviewed Jul 19, 2019
View reviewed changes
jtattermusch
jtattermusch reviewed Jul 19, 2019
View reviewed changes
csharp/src/Google.Protobuf/ExtensionSet.cs
IExtensionValue value;
if (first.ValuesByNumber.TryGetValue(pair.Key, out value))
{
value.MergeFrom(pair.Value);
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@ObsidianMinor ObsidianMinor Jul 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, is that not the correct behavior for merging?

Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure. @anandolee would know better?

jtattermusch
jtattermusch reviewed Jul 23, 2019
View reviewed changes
@Dig-Doug Dig-Doug mentioned this pull request Dec 21, 2019
Closed
@Dig-Doug Dig-Doug mentioned this pull request Jan 1, 2020
Closed
@jtattermusch jtattermusch mentioned this pull request Apr 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anandolee anandolee anandolee left review comments

@jskeet jskeet Awaiting requested review from jskeet

+1 more reviewer

@jtattermusch jtattermusch jtattermusch left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@anandolee anandolee

Labels

c# cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ObsidianMinor @anandolee @jskeet @lostindark @jtattermusch @acozzette @TeBoring @googlebot @kokoro-team