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.

Sign up for GitHub

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

Jump to bottom

extra::json: Remove or rework ToJson #9028

Closed
bluss opened this issue Sep 6, 2013 · 5 comments
Closed

extra::json: Remove or rework ToJson #9028

bluss opened this issue Sep 6, 2013 · 5 comments

Comments

@bluss
Copy link
Member

bluss commented Sep 6, 2013

There are two ways to encode values into json in string form, and they can provide different results or be wrong:

  • ToJson transforms a rust value to Json, which can be encoded by a serialize::Encoder
  • json::Encoder implements the Encoder trait, so you can give it arbitrary values, as long as they implement Encodable, and it will output json, or json-like text.

We need to provide a single method of encoding to Json, and make sure we use serialize correctly if we use it.

The ToJson trait expresses the restriction that only maps with string keys can be encoded to Json. This is easy to step around by using the other json encoding method, which results in invalid json.
@bluss bluss mentioned this issue Sep 6, 2013
Closed
@JeffMurray
Copy link

I use to_json() a lot and find it quite handy when packaging up rust native data types for Json streaming.

I think if extra::json::Encoder ( and PrettyEncoder ) could be limited to encoding Json values the problem would be solved.

Other than that I find both the encoders pretty useful.

Thanks.

@brandonson brandonson mentioned this issue Sep 12, 2013
Closed
@brandonson
Copy link
Contributor

I think this may just point to a need for clarification in the docs. I'd expect ToJson to perform a conversion to json that creates a very specific representation of whatever type implements it. On the other hand, I expect Encoder to perform a conversion that creates a more general representation which is therefore available to more types.

Unfortunately, the difference is not very clear in the docs, and ToJson and Encoder/Decoder were indeed very intertwined, and may still be.

@bluss
Copy link
Member Author

bluss commented Sep 12, 2013

I think extra::json is abusing the serialization API. Json implements Encodable, but not Decodable. The impl of Encodable is not valid in serialization context, since it does not emit the enum tag. This abuse must be eith reworked, or hidden inside the module (by wrapping it in a private newtype?)

@flaper87
Copy link
Contributor

flaper87 commented Feb 5, 2014

Triage bump. By checking the code, this is still the case. Decodable doesn't seem to be implemented and ToJson hasn't been reworked.

This issues seems legit to me. I don't think removing ToJson is the way to go, I'd opt for re-factoring it.

@mrec mrec mentioned this issue Jun 26, 2014
Merged
bors added a commit that referenced this issue Jun 26, 2014
@bors
auto merge of #15192 : mrec/rust/json-nan, r=alexcrichton
fa63b8c 
The JSON spec requires that these special values be serialized as "null"; the current serialization breaks any conformant JSON parser. So encoding needs to output "null",  `to_json` on floating-point types can return `Null` as well as `Number` values, and reading a `Null` value when specifically expecting a number should be interpreted as NaN. There's no way to round-trip Infinity through JSON.

This is my first attempt at both writing Rust and opening pull requests, so please dial your derp detector up to eleven when reviewing. A `rustc --test lib.rs` in `libserialize` passes all tests; a `make check` of the whole tree fails with the error below, but it doesn't look obviously related and the docs say that `make check` is known to be flaky on Windows.

    ---- [compile-fail] compile-fail/svh-change-significant-cfg.rs stdout ----
            task '[compile-fail] compile-fail/svh-change-significant-cfg.rs' failed at 'called `Result::
    unwrap()` on an `Err` value: couldn't create file (end of file (unknown error); path=i686-pc-mingw32
    \test\compile-fail\svh-a-base.err; mode=truncate; access=write)', C:\msys\home\Mike\rust\src\libcore
    \result.rs:545

Incidentally, it may just be my lack of familiarity with the language and its idioms, but the duplication between `Encoder`/`PrettyEncoder` had a distinct code smell to it. The size of the file (~3500 lines) also made it a bit hard to navigate. Has there been any discussion of refactoring and/or breaking it up? I couldn't find anything in Issues except the ancient #9028.
@oli-obk oli-obk mentioned this issue Dec 3, 2014
Merged
bors added a commit that referenced this issue Dec 23, 2014
@bors
auto merge of #19490 : oli-obk/rust/json_non_string_key_maps, r=alexc…
440271a 
…richton

importing object type string key maps is still supported
writing them should be explicit, and can be done as follows

```rust
let some_tree_map : TreeMap<String, Json> = ...;
Json::Object(some_tree_map).to_writer(&mut writer);
```

related to #8335, #9028, #9142
bors added a commit that referenced this issue Jan 19, 2015
@bors
Auto merge of #19490 - oli-obk:json_non_string_key_maps, r=alexcrichton
7f8c687 
importing object type string key maps is still supported
writing them should be explicit, and can be done as follows

```rust
let some_tree_map : TreeMap<String, Json> = ...;
Json::Object(some_tree_map).to_writer(&mut writer);
```

related to #8335, #9028, #9142
@steveklabnik
Copy link
Member

Encoding has been moved out, and we've had some related improvements since then. I'm giving this a close. If you feel some of this is still important, please open it on https://github.com/rust-lang/rustc-serialize

flip1995 pushed a commit to flip1995/rust that referenced this issue Jun 30, 2022
@bors
Auto merge of rust-lang#9028 - kyoto7250:issue_8798, r=xFrednet
6985e65 
confirm  using chain in collapsible_span_lint_calls

close rust-lang#8798

This PR fixes false positive when using chain in `collapsible_span_lint_calls`.

changelog: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@flaper87 @steveklabnik @brandonson @bluss @JeffMurray