Skip to content

Conversation

@krakow10
Copy link

@krakow10 krakow10 commented Jul 8, 2025

This allows the ListValue, Struct, and Value types to be serialized with serde by enabling a new "serde" crate feature.

This code was originally written by ChatGPT (and me) in July 2024, back when I would not have been able to write it entirely myself. It had dependency issues and a strange nested Visitor implementation, but it worked well enough to use privately. I've learned a lot of Rust in a year, and went over it today and was able to cut out 70 lines of ai cruft nonsense and use a crate feature. The reason I'm upstreaming this now is because I saw that the "arbitrary" feature was added which looks similar to serde. I briefly tested this in my game's staging deployment and it seems to work well enough to not cause any new issues compared to the previous crufted ai implementation.

Also of interest: Add #[derive(serde::Serialize, serde::Deserialize)] to protobuf generated structs

Copy link
Contributor

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. This looks like a good first step for a JSON implementation. I would like to see the following additions:

  • Describe the serde feature flag in the lib.rs doc comment. This has to at least say that not all types are serializable
  • Please add some tests to prevent future regressions
  • Can you confirm that this serialization follows the official ProtoJSON format? It would be super if you could confirm that in a test case.

@krakow10
Copy link
Author

Thank you for your contribution. This looks like a good first step for a JSON implementation. I would like to see the following additions:

* Describe the `serde` feature flag in the `lib.rs` doc comment. This has to at least say that not all types are serializable

I've added a description. Please let me know if it's insufficient.

* Please add some tests to prevent future regressions

Done.

* Can you confirm that this serialization follows the official [ProtoJSON format](https://protobuf.dev/programming-guides/json/)? It would be super if you could confirm that in a test case.

Eyeballing it, it seems like it won't make something invalid for serialization, but for deserialization it looks like converting {} -> protobuf.Empty is not possible with the current types. The current behaviour is to produce an empty Value::StructValue. I'm not sure how to approach adherence to the specification in a testable way.

@krakow10 krakow10 requested a review from caspermeijn July 15, 2025 08:43
@krakow10
Copy link
Author

@caspermeijn Is this sufficient? Note that I've added a line in the config that ignores the unused dependency false positive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants