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

Add support for Format::Any #47

Closed
wants to merge 6 commits into from

Conversation

JonathanBrouwer
Copy link

Summary

Hey there! I'm aware that the serde-reflection crate was developed in the context of serde-generate, but I'm using it for a different purpose, which is generating an XML schema for my serde-encoded types.

For this purpose I need to be able to detect types that require self-describing data structures, so I added a new variant to Format for this. I'm aware this is a breaking change and that you earlier rejected this proposal in #33, but I thought I'd try anyways, since otherwise I need to maintain my fork.

Feel free to propose changes to my implementation or reject it outright if you feel this doesn't fit the crate.

match registry.get("Thing").unwrap() {
ContainerFormat::Struct(fields) => {
assert_eq!(fields.len(), 2);
assert_eq!(fields[0].value, Format::Any);
Copy link
Contributor

@ma2bd ma2bd Sep 27, 2024

Choose a reason for hiding this comment

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

With a little bit of boilerplate in user code, we could have Format::Typename("Value") here.

Specifically, there could be a wrapper struct around serde_json::Value (say struct Value(serde_json::Value)) so that the (de)serialize trait implementations of the new type are treating it as a plain enum (hence compatible with the current framework of serde-{reflection,generate}) for non-human-readable (de)serializers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or alternatively use #[serde(with = "module")] instead of a wrapper type (doc).

Here is an example of conditional implementation of (de)serialize traits:
https://github.com/linera-io/linera-protocol/blob/152ebf5fb767ae967b381d3ed3b64dd9aedc8d9e/linera-base/src/data_types.rs#L60

Copy link
Author

@JonathanBrouwer JonathanBrouwer Sep 27, 2024

Choose a reason for hiding this comment

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

If I understand correctly, this would indeed work perfectly. The problem is that the generated XML (which is the serde data format I need to use, sadly :p) would be less readable, explicitly encoding whether each field is a Value::Int, Value::String, etc. You'd get XML like this

<person>
  <object>
      <name><string>Jonathan</string></name>
      <age><number>23</number></age>
  </object>
</person>

Instead of

<person>
    <name>Jonathan</name>
    <age>23</age>
</person>

To clarify my goal, I'm building a new serde data format crate which can generate XML and outputs a corresponding XSD (xml schema) using the serde-reflection crate to find the structure of the serialized type. I want to handle types that require a self-describing data format gracefully during the generation of the XSD, emitting a wildcard for that part

Copy link
Author

Choose a reason for hiding this comment

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

Or alternatively use #[serde(with = "module")] instead of a wrapper type (doc).

Here is an example of conditional implementation of (de)serialize traits: https://github.com/linera-io/linera-protocol/blob/152ebf5fb767ae967b381d3ed3b64dd9aedc8d9e/linera-base/src/data_types.rs#L60

Oh I see, with conditional implementation of serialize/deserialize this could indeed work. Bit of a hacky solution, but I could try doing it that way

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that replacing serde_json::Value or annotating the fields could be annoying but for the rest what prevents you from obtaining the desired generated output once you have a format with a recognizable typename?

Copy link
Author

Choose a reason for hiding this comment

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

Currently my code was hitting the Err::NotSupported case in this crate's deserialize implementation, therefore I'm not getting a Registry at all. The example you sent above where a serialize/deserialize is switching its implementation based on the is_human_readable flag would be a workaround for this problem, so the deserialize_any call is not triggered and then I could manually filter out the types that use deserialize_any. This is a bit less nice to use than the implementation in this PR but an acceptable compromise

Copy link
Contributor

@ma2bd ma2bd Sep 27, 2024

Choose a reason for hiding this comment

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

Yes. Obviously, you can pick the solution that works for you. Given that there is a workaround, though, I'm leaning towards keeping the current codebase as it is.

Copy link
Author

Choose a reason for hiding this comment

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

That's fair enough, then I'll close this PR, thanks a lot for the review!

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