Skip to content

serialize: impl ToJson for all Encodables #12936

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

Closed
wants to merge 1 commit into from

Conversation

seanmonstar
Copy link
Contributor

I just wanted to submit this at this point to see if something like this would be accepted. If so, I can clean it up some more, such as converting to Vec<T>, etc.

The short is that this is now possible:

#[deriving(Encodable)]
struct Foo {
  bar: uint
}

foo.to_json(); // => json::Object(TreeMap)

fixes #8335

@alexcrichton
Copy link
Member

I'm a little sad to see yet another encoder in the json module. I would expect there to be one encoder, not three. Two is already pushing it a little bit. I fear that the json modules needs to be thought out before tacking on more functionality.

From what it looks like, the are three different representations that the json module deals with: rust values, strings, and values of type Json. The current module implements

from to implemented
rust json no
rust string yes (Encoder)
json string yes (Encoder)
json rust yes (Decoder)
string rust no
string json yes (Parser)

This is a bit of a hefty matrix to deal with, and I fear the utility of the Json type is diminished due to how it's used. In the ideal world, I would expect the json module to contain:

  • One encoder, which is an implementation of the Encoder trait.
  • One decoder, which is an implementation of the Decoder trait.
  • A Json enum which implements encodable/decodable

I would imagine that the decoder is what the parser is today, and that this intermediate format of Json isn't used that often. Instead it's only used if the format being deserialized isn't known ahead of time.

What do you think of this?

@seanmonstar
Copy link
Contributor Author

Personally, I was surprised to read in #8335 the need to turn a rust object into a Json, but not all the way to a string. Even the name of the trait, ToJson, gives me pause. I think of JSON as a string. Always. If it's not a string, it's something else. So, I assume ToJson.to_json() to return a string.

I also think the Encoder and PrettyEncoder could be just one encoder, with a few ifs in the methods. If that's preferred, I can clean up this module that way.

@alexcrichton
Copy link
Member

That sounds good to me! Here's a few things that I think would work well:

  • Merging Encoder and PrettyEncoder with a flag (as you suggested)
  • Removing ToJson
  • Delete Decoder and rename Parser to Decoder (folding all functionality into one another)
  • Keep the Json enum, implementing Encodable/Decodable for it

That way you use the Decoder to go from a string to a rust value and the Encoder to go from a rust value to a string. Conversion between rust values and a Json value would require going through a string. You can also always decode directly into a Json value if you need to inspect the object.

@seanmonstar
Copy link
Contributor Author

Why remove the Parser? Looking in ebml, theres a parser-like object, and the decoder is separate.

@alexcrichton
Copy link
Member

ebml isn't necessarily the gold standard for libraries, and the Parser's functionality would remain, it would just live under a new name.

@seanmonstar
Copy link
Contributor Author

is there any need to keep the Json enum around? Why not decode straight from a string?

If it does stick around, I can't figure out how I would implement Decodable for Json. There doesn't seem to be any way to get a hint of the next value. Can I use some matching? Or use the Any trait somehow?:

impl<D: Decoder> Decodable<D> for Json {
  fn (d: &mut D) -> Json {
    // can i somehow match against types?
    match Decodable::decode(d) {
      f64 => Number,
      // etc?
    }
  }
}

@alexcrichton
Copy link
Member

Keeping Json around seems useful because you can deserialize an arbitrary blob into it.

You don't need to implement <D: Decoder> Decodable<D>, you can implement Decodable<JsonDecoder>.

}
}

fn into_json(&mut self) -> Json {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be to_json instead of into_json, or instead it should consume self. We've standardized on using into_ to represent methods that consume self to transform into the result`. Also, this should return an error type instead of failing.

@seanmonstar
Copy link
Contributor Author

I'll open a new PR of json cleanup. I've also found the error handler in Encoder's to unfun, so I might fix #12292 before.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 27, 2024
…r_comparison, r=Alexendoo

Add MSRV for manual_pattern_char_comparison

Fixes rust-lang#12936

changelog: [`manual_pattern_char_comparison`]: Add MSRV 1.58
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.

extra::json doesn't support using Encodable to encode straight to a json value
3 participants