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

Implement Serialize and Deserialize #21

Closed
wants to merge 1 commit into from

Conversation

lawliet89
Copy link

@lawliet89 lawliet89 commented Jul 15, 2017

For UniCase and Ascii

Serialization was relatively straightforward, but deserialization was a bit tricky to work with. I elected to implement two versions of it for owned and borrowed data.

I also had to remove test support for Rust 1.3.0.

Please let me know if the documentation is too extraneous, or if any of the implementation could be improved.

@lawliet89 lawliet89 force-pushed the serde branch 6 times, most recently from 813c017 to 5d7928e Compare July 15, 2017 17:01
@seanmonstar
Copy link
Owner

Um, wow. That's a lot of great work, with documentation and extensive tests too!

Which makes me feel so bad when I say this, but, I'm not certain that I'd want to pull in the extra dependency here... Anyone could serialize their stuff with serde's #[serde(with)] attributes, just saying that any UniCase or Ascii can be serialized like a String (or well, like T).

@lawliet89
Copy link
Author

lawliet89 commented Jul 16, 2017

It was a good exercise in practicing with traits and lifetimes.

I've gated the extra dependency behind a feature gate. Would that be acceptable?

I've thought of implementing this as an external crate and then have the user use it with with (like url_serde) but it would not be usable if your struct has something like HashMap<UniCase<String>> because you cannot tell serde (AFAIK) to use a custom (de)serializer for the inner type. So for the url_serde crate, I ended up defining a wrapper struct around Url to workaround this. It's not very satisfactory IMO and extra work.

If you disagree, I'll just wrap this PR up in another crate and do that instead.

@seanmonstar
Copy link
Owner

I've gated the extra dependency behind a feature gate. Would that be acceptable?

The problem I've had in the past is that by exporting traits like these, it's a burden if the traits ever change. I realize that with serde at 1.0, they probably won't break for a long time, but I'm still in the (admittedly smaller) camp of "let a separate crate provide this" (or have serde grow the ability to tell it about inner types).

@lawliet89
Copy link
Author

lawliet89 commented Jul 19, 2017

Sure. I have moved this into a separate crate instead.

I'll close this PR for now.

EDIT for future readers: Asking serde about this issue in serde-rs/serde#993.

@lawliet89 lawliet89 closed this Jul 19, 2017
@tecywiz121
Copy link

Over a year has passed since this PR was closed, and I think it's time to revisit it.

Not having support for serializing HashMap<UniCase, V> is pretty significant, since I can't imagine very many cases where you'd have UniCase outside of a collection.

Regarding your concern about the Serialize and Deserialize traits changing, that would require a new major version and therefore a different feature flag in unicase. Supporting both would just mean adding the implementations, and anyone could do that if it's ever required.

@lawliet89 lawliet89 mentioned this pull request Jul 14, 2020
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.

3 participants