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

Generate protobufs ourselves using protobuf-net.Reflection #686

Merged
merged 30 commits into from
Oct 19, 2019

Conversation

yaakov-h
Copy link
Member

Another approach to #628.

This assumes/ignores the [DefaultValue] problem. I think if we can get that upstreamed, this will be fine.

Note that:

  • I'm just outright ignoring the "hey you didn't specify proto2 or proto3" warning.
  • This removes XmlSerializer attributes
  • This removes [Serializable]
  • This removes EnumPassthru, though I don't know if that is still needed or not.

@yaakov-h yaakov-h changed the title Generate protobufs ourselves using protobuf-net.Reflectrion Generate protobufs ourselves using protobuf-net.Reflection May 23, 2019
@yaakov-h
Copy link
Member Author

The DefaultValue bug has been fixed (protobuf-net/protobuf-net#516) but I have no idea if/when that code will be released.

I guess compiling protobuf-net ourselves (without any custom patches) wouldn't be so bad...

@yaakov-h
Copy link
Member Author

yaakov-h commented May 24, 2019

With these change, _ characaters are being dropped in enum names, e.g. k_Foo becomes just kFoo.

@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #686 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #686   +/-   ##
=======================================
  Coverage   22.44%   22.44%           
=======================================
  Files          96       96           
  Lines        9674     9674           
  Branches      817      817           
=======================================
  Hits         2171     2171           
  Misses       7370     7370           
  Partials      133      133
Impacted Files Coverage Δ
...team/Handlers/SteamMatchmaking/SteamMatchmaking.cs 3.77% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 145acfe...ecc53b6. Read the comment docs.

@yaakov-h
Copy link
Member Author

_ characters are no longer being dropped after updating to latest (alpha) protobuf-net.Reflection.

The flip side is that enum values have exactly the same name as in the protobuf, so many are now SCREAMING_SNAKE_CASE as per the original protos.

This PR now relies on protobuf-net/protobuf-net#518.

@xPaw xPaw mentioned this pull request May 24, 2019
2 tasks
@@ -1,21 +1,21 @@
enum GCProtoBufMsgSrc {
Copy link
Member

@xPaw xPaw May 24, 2019

Choose a reason for hiding this comment

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

This file changed for no reason (line endings I assume?)

This also affected the csv file and the powershell script.

Copy link
Member Author

Choose a reason for hiding this comment

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

I only moved the files... possibly something in my git install tweaked the line endings?

@xPaw
Copy link
Member

xPaw commented May 24, 2019

This looks like it achieves the same result as my patch approach, except by extending protobuf.net, so that's good.

The only missing change would be forced default to proto2 version, instead of leaving it null, I am unsure if this actually affects any of the generated output.

Copy link
Member

@xPaw xPaw left a comment

Choose a reason for hiding this comment

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

Tested this by running SteamDB, it builds and runs as expected.

@yaakov-h yaakov-h marked this pull request as ready for review October 15, 2019 22:46
@yaakov-h yaakov-h added this to the 2.3.0 milestone Oct 15, 2019
@yaakov-h
Copy link
Member Author

Tested and verified that EnumPassthru is no longer required.

@yaakov-h yaakov-h merged commit 76ad810 into master Oct 19, 2019
@yaakov-h yaakov-h deleted the experiment/protobuf-gen branch October 19, 2019 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants