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

Impl Serialize/Deserialize for std::ops::{Bound, RangeFrom, RangeTo, RangeToInclusive} #1466

Merged
merged 3 commits into from
Feb 2, 2019

Conversation

0nkery
Copy link
Contributor

@0nkery 0nkery commented Jan 29, 2019

Implemented Serialize and Deserialize traits for various interval-related types from std.

Could help me to setup cfgs etc properly?

@dtolnay
Copy link
Member

dtolnay commented Jan 30, 2019

Could you explain more about your use case for these impls? How is all of this going to be used in what you are building?

@0nkery
Copy link
Contributor Author

0nkery commented Jan 30, 2019

Hi. It's useful because Diesel uses (Bound<T>, Bound<T>) to represent Postgres ranges. We use one struct for queries and JSON ser/de so it seems like a good thing to have.

As for Range-types, I find it appropriate to use precise types for ranges (both with Diesel and Serde). Currently I propose changes to Diesel (diesel-rs/diesel#1971) to add the ability to use these types with Queryable.

Anyway, if you think that Range-types should not be added, I'm glad to revise PR.

@dtolnay
Copy link
Member

dtolnay commented Feb 1, 2019

It sounds like the two reasons for wanting the Bound impls is Diesel's choice of Bound to represent ranges and your code's choice of doing queries and json based on the same struct. What would be the impact on your code if we decide not to provide these impls in Serde? Probably you would need to apply a serialize_with / deserialize_with function on the field containing Bound right?

@0nkery
Copy link
Contributor Author

0nkery commented Feb 1, 2019

Yep, that's right.

@dtolnay dtolnay merged commit 7a72b4c into serde-rs:master Feb 2, 2019
@dtolnay
Copy link
Member

dtolnay commented Feb 2, 2019

Thanks, I merged the PR but pared it down to just the Bound impls in a later commit. I am not opposed to the other impls but I would prefer not to accept code that I don't know of anyone wanting to use.

Published in 1.0.86.

@0nkery
Copy link
Contributor Author

0nkery commented Feb 2, 2019

Thanks! We've decided to use Bound only anyway.

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.

2 participants