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

Add serialization deserialization #43

Merged
merged 30 commits into from
Oct 9, 2024

Conversation

dcampbell24
Copy link
Contributor

This is the general idea. I can make load generic like I did with save. With a struct that implements Serialize and Deserialize it is easy to add other implementations besides JSON. I am partial to RON, and you mentioned a curl cookie format. Let me know what you think and what I have to change to get this included, along with what tests you expect.

I didn't reuse more code than I did, because the old code was line oriented, but this Serializes and Deserializes the whole blob at once.

I am creating a pull request, but I expect some work to be done before a merge happens.

@dcampbell24
Copy link
Contributor Author

I made load generic.

@dcampbell24
Copy link
Contributor Author

Sorry about making so many commits, but this is ready for review.

@pfernie
Copy link
Owner

pfernie commented Aug 23, 2024

Thanks for the work here; I haven't had a chance to do a proper review, so just touching this to let you know I've seen it and hope to have a chance to look it over soon.

@dcampbell24
Copy link
Contributor Author

Okay thanks. no rush.

The original implementation of `cookie_store` provided convenience
methods for saving/loading via JSON. The output format is not valid
JSON, however; `serde::json` introduces proper support. The legacy
implementation is retained alongside the new functionality; we provide
guidance on the difference via documentation.
@pfernie
Copy link
Owner

pfernie commented Oct 8, 2024

I had some time to review this. I did a bit of a refactor; I introduced mod serde::{json,ron} and moved the relevant implementations there. I also feature gated the functionality, to avoid unnecessary dependencies. The serde_Json feature is currently enabled by default to avoid surprises, but a release will have to be a major ("0."-style, so 0.22.0) version bump anyway, so it might be better to take the opportunity and not enable it by default and avoid pulling in serde when not wanted.

I also dropped the intermediate CookieStoreSerialized struct, and just serialize/deserialize through a Vec<>, to avoid the extra level of nesting. IDK if that still suits your purpose; the only reason I'd see not to take this approach is if we anticipated adding more fields to the CookieStore that we'd want to serialize (I currently don't have any such plans).

Finally, as commented before, I don't want to fully remove the existing CookieStore::save_json and co. The new approach is IMO better, but if there are users out there (such as myself...) with "old" style cookie stores serialized, the new methods won't work with them since they are not actually valid JSON. I've introduced documentation around it to highlight the difference b/w the old methods and the new serde::Json module, as well as marking them deprecated to discourage their use.

LMK if this is still in line with the functionality you were intending.

@dcampbell24
Copy link
Contributor Author

Yeah, it's good! Thanks for your work.

@pfernie pfernie merged commit 3d4c286 into pfernie:master Oct 9, 2024
1 check passed
@dcampbell24 dcampbell24 deleted the add-serialization-deserialization branch October 9, 2024 21:33
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.

2 participants