Feature request: Return `Difference` instead of String for `assert_json_matches_no_panic`

[B-2U]

Hi there,
first of all, thank you for forking serde-json-diff and add the epsilon floats comparison!

I just came across this crate after forking serde-json-diff a few days ago , and I would prefer to use this one instead of reinventing the wheel myself.

One of my use case is to expose and return the Difference<'> in the assert_json_matches_no_panic() function.
This would allow me to perform further processing with the diffs.

However, this raised a lifetime problem. I created an issue with details in my own repository: B-2U/assert-json-same#4.

TLDR:

1. Make Difference an owned type => slow down the crate completely
2. Change the function signature so it take two serde_json::Value ownership instead => it break the flexibility of the api
3. Current approach, introduce the owned types variants DifferenceBuf, PathBuf and KeyBuf, like str/String and Path/PathBuf

So I wonder would you prefer any of them?

Thx!

[hardselius]

Hello!

Thank you for wanting to contribute. I think option 3 might be the cleanest addition, and option 1 behind it. Feel free to submit a PR, but I would like to see more extensive tests of the new types. Maybe add in some doctests with some relevant use-cases?

[B-2U]

Hello!

Thank you for wanting to contribute. I think option 3 might be the cleanest addition, and option 1 behind it. Feel free to submit a PR, but I would like to see more extensive tests of the new types. Maybe add in some doctests with some relevant use-cases?

Got it!
I'll let you know and submit a PR once I complete the tests and doctests!

[hardselius]

@B-2U btw, did you see this PR davidpdrsn/assert-json-diff#35? Could that solve your problem? If you just expose the diff function, you could use that instead of assert_json_matches_no_panic. The latter will basically just check that there's no diff. It's much less work and much less code, which is good.

[B-2U]

@B-2U btw, did you see this PR davidpdrsn/assert-json-diff#35? Could that solve your problem? If you just expose the diff function, you could use that instead of assert_json_matches_no_panic. The latter will basically just check that there's no diff. It's much less work and much less code, which is good.

Yeah I did see that, it's really attractive since it needs basically no modification.
What my consideration is

1. It takes serde_json::Value instead of anything Serializable, like Option2 which is less flexible
2. And since we're exposing it, it's pbbly better to keep all API parameters the same

[hardselius]

Maybe you can wrap the existing diff function in a public one that takes arguments that are Serializeable?

[B-2U]

Maybe you can wrap the existing diff function in a public one that takes arguments that are Serializeable?

Yes, it's exactly what I'm doing
My plan is creating a sibling pub fn like the assert_json_matches_no_panic()
But since it takes something Serializable in, it cannot directly return a Difference<'_> which relies on the serialized Value (which live in the function) that's going to be dropped, So we'll need a owned version of Difference<'_> which bring us back to option 3 :(

[B-2U]

Ah I forgot the push the commit on my laptop to my fork, I'll do it tomorrow and maybe make a draft PR first 👍

[hardselius]

Maybe you can wrap the existing diff function in a public one that takes arguments that are Serializeable?

Yes, it's exactly what I'm doing My plan is creating a sibling pub fn like the assert_json_matches_no_panic() But since it takes something Serializable in, it cannot directly return a Difference<'_> which relies on the serialized Value (which live in the function) that's going to be dropped, So we'll need a owned version of Difference<'_> which bring us back to option 3 :(

Aha. I see. I did try it out too, out of curiosity and you are, of course, correct. Then I tried something like this

#[derive(Debug)]
pub struct Diff {
    lhs: Value,
    rhs: Value,
}

impl Diff {
    pub fn new<Lhs, Rhs>(lhs: Lhs, rhs: Rhs) -> Self
    where
        Lhs: Serialize,
        Rhs: Serialize,
    {
        let lhs = serde_json::to_value(lhs).unwrap_or_else(|err| {
            panic!(
                "Couldn't convert left hand side value to JSON. Serde error: {}",
                err
            )
        });
        let rhs = serde_json::to_value(rhs).unwrap_or_else(|err| {
            panic!(
                "Couldn't convert right hand side value to JSON. Serde error: {}",
                err
            )
        });
        DiffBuf { lhs, rhs }
    }

    pub fn diff<'a>(&'a self, config: &'a Config) -> Vec<Difference<'a>> {
        diff(&self.lhs, &self.rhs, config)
    }
}

but I don't like the ergonomics of that approach.