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 serialize / deserialize features. #65

Merged
merged 8 commits into from
Nov 13, 2021
Merged

Add serialize / deserialize features. #65

merged 8 commits into from
Nov 13, 2021

Conversation

BratSinot
Copy link
Contributor

@BratSinot BratSinot commented Nov 10, 2021

#46

Greetings,

Realization is optional (through features). For me such feature needed to get rid of tons of manual Deserializations / Serializations like this:

use arc_swap::ArcSwap;

#[derive(Debug, serde::Serialize, serde::Deserialize)]
struct Foo {
    field0: ArcSwap<usize>,
    field1: ArcSwap<String>,
}

fn main() {
    let data = Foo {
        field0: ArcSwap::from_pointee(123),
        field1: ArcSwap::from_pointee("FOO".to_owned()),
    };

    println!("{}", serde_json::to_string(&data).unwrap());
}

@vorner
Copy link
Owner

vorner commented Nov 10, 2021

#46 was waiting for someone just like you, having the motivation to get around to this :-). I'll give it a proper read later today and see what the CI says.

I just wonder if two features are needed (while they both bring serde in, which is the big chunk), or if single serde one would be enough.

Copy link
Owner

@vorner vorner left a comment

Choose a reason for hiding this comment

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

There are few little nitpicks, and I'd prefer having only one feature instead of two ‒ it doesn't really add that much compilation time to add the one extra trait implementation, once the whole serde is already brought in.

Also, I've noticed github changed the way actions are run on pull requests and had to enable something more on master to run them. Would it be possible for you to rebase this on top of the newest master, so the tests are run?

Thanks!

src/serde.rs Outdated Show resolved Hide resolved
src/serde.rs Outdated Show resolved Hide resolved
src/serde.rs Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2021

Codecov Report

Merging #65 (3cfc699) into master (13e5e3f) will increase coverage by 0.68%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   85.56%   86.25%   +0.68%     
==========================================
  Files          17       18       +1     
  Lines        1032     1091      +59     
==========================================
+ Hits          883      941      +58     
- Misses        149      150       +1     
Impacted Files Coverage Δ
src/lib.rs 94.81% <ø> (-0.31%) ⬇️
src/serde.rs 100.00% <100.00%> (ø)

Copy link
Owner

@vorner vorner left a comment

Choose a reason for hiding this comment

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

OK, I'm mostly happy about it. Clippy and rustfmt aren't, and the history is a bit cluttered. Is it fine with you if I just take it over and deal with these "formalities" (I'll be dealing with a release anyway)?

@BratSinot
Copy link
Contributor Author

BratSinot commented Nov 12, 2021

OK, I'm mostly happy about it. Clippy and rustfmt aren't, and the history is a bit cluttered. Is it fine with you if I just take it over and deal with these "formalities" (I'll be dealing with a release anyway)?

Yeah, of course.

I saw you using nightly? Maybe this make some difference. I also use Clippy and rustfmt (by default in my fav IDE), maybe there was some kind of glitch in my IDE.

Ow, it wasn't IDE glitch, it was IDE old settings "glitch" =)

@vorner vorner merged commit 8d6b1fb into vorner:master Nov 13, 2021
@BratSinot BratSinot deleted the serde branch November 14, 2021 09:37
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