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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add impl From<()> for Value #585

Merged
merged 1 commit into from Nov 24, 2019

Conversation

Nilix007
Copy link
Contributor

Hey there,

this tiny PR adds the conversion from () to Value::Null.

I feel this is justified as serde_json already perceives () as the JSON value null:

println!("{:?}", serde_json::from_str::<()>("null"));
// prints `Ok(())`
    
println!("{:?}", serde_json::to_string(&()));
// prints `Ok("null")`

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0eac935756f3537acdf45486dcabb4db

Cheers,
Felix

PS: Please have a look at the marvelous signature of <Value as From<()>>::from 馃槅

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Can you share an example where having this impl would be helpful?

@Nilix007
Copy link
Contributor Author

Nilix007 commented Nov 19, 2019

Mh. So I was working on a library which uses diesel's Jsonb internally for some kind of dynamic data field. As Jsonb maps directly to Value, I'm exporting functions like fn do_something<T: Into<Value>>(additional_data: T) {} so that I can avoid pulling in the complete serde dependency tree and skip the serialization step completely (yay, one error case less on my side due to type safety).

Anyway, long story short, I think that the conversion from this PR would be quite convenient for the end user of my lib. And as it kinda feels "natural" to me, I sent this PR.

@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2019

What do you mean by "pulling in the complete serde dependency tree"? If you depend on serde_json to use Value, that depends on serde anyway.

Do you prefer Into<Value> over:

fn do_something(additional_data: impl Serialize)

which would be more flexible?

@Nilix007
Copy link
Contributor Author

What do you mean by "pulling in the complete serde dependency tree"? If you depend on serde_json to use Value, that depends on serde anyway.

Err, you're right, of course. Please ignore this part 馃槵

Do you prefer Into over:

fn do_something(additional_data: impl Serialize)

which would be more flexible?

Yes, because serde's serialization (and serde_json::to_value respectively) can fail while From/Into cannot.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay dtolnay merged commit bf8cc66 into serde-rs:master Nov 24, 2019
@dtolnay
Copy link
Member

dtolnay commented Nov 24, 2019

Published in 1.0.42.

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

Successfully merging this pull request may close these issues.

None yet

2 participants