Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix C# generator handling of a field named "none" in a oneof #9636

Merged
merged 4 commits into from
Mar 30, 2022

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Mar 16, 2022

Fixed #9605.

(Review one commit at a time; the final commit is just generated.)

@jskeet
Copy link
Contributor Author

jskeet commented Mar 28, 2022

Note: the test protos are generated; I can easily regenerate them after rebasing, when this is approved.

Copy link

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
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.

LGTM.

@jskeet
Copy link
Contributor Author

jskeet commented Mar 29, 2022

I was going to wait until #9686 was in, but I'll do that separately. Will regenerate (to update testprotos.pb) and merge.

@jskeet
Copy link
Contributor Author

jskeet commented Mar 29, 2022

Have actually found some more places I wasn't fixing up - will fix those and request a re-review.

Copy link

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

LGTM still!

@jskeet
Copy link
Contributor Author

jskeet commented Mar 30, 2022

Thanks Amanda. Will rebase again, look into the MacOS changes, and then merge if it's green.

jskeet added 2 commits March 30, 2022 13:45
When following the instructions directly (for developing on Windows) we should end up with a generator that's easy to run for csharp/generate_protos.sh, and we shouldn't end up trying to stage the build output.
jskeet added 2 commits March 30, 2022 15:39
This doesn't test *all* possibilities (e.g. the field being a
wrapper, or a message field, etc) - but I'm fairly confident that I
found all the places referring to the case.
@jskeet jskeet force-pushed the oneof-none-csharp branch from 99fad02 to 10e1fed Compare March 30, 2022 14:40
@jskeet jskeet removed the kokoro:run label Mar 30, 2022
@jskeet jskeet merged commit 9e6e4eb into protocolbuffers:main Mar 30, 2022
@jskeet jskeet deleted the oneof-none-csharp branch March 30, 2022 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C#] oneof cannot have a field named none
5 participants