benhall-7 / diff-struct

Rust trait for diffing and applying diffs between data structures
MIT License
54 stars 14 forks source link

Incorrect handling of optional fields with String #7

Open ghostinushanka opened 2 years ago

ghostinushanka commented 2 years ago

Hello @BenHall-7 ,

Playing with the lib I've come across inconsistent behavior for structs containing String types. Example:

use diff::Diff;

#[derive(Debug, PartialEq, Diff)]
#[diff(attr(
    #[derive(Debug, PartialEq)]
))]
struct OptionalInteger {
    x: i32,
    ox: Option<i32>,
}

#[derive(Debug, PartialEq, Diff)]
#[diff(attr(
    #[derive(Debug, PartialEq)]
))]
struct OptionalString {
    x: String,
    ox: Option<String>,
}

fn main() {
    let empty_integer = OptionalInteger { x: 42, ox: None };
    let filled_integer = OptionalInteger { x: 42, ox: Some(42)};
    let di = empty_integer.diff(&filled_integer);
    println!("{:#?}", di);

    let empty_string = OptionalString { x: "42".to_string(), ox: None};
    let filled_string = OptionalString { x: "42".to_string(), ox: Some("42".to_string())};
    let ds = empty_string.diff(&filled_string);
    println!("{:#?}", ds);
}

Expected output:

OptionalIntegerDiff {
    x: 0,
    ox: Some(
        42,
    ),
}
OptionalStringDiff {
    x: None,
    ox: Some(
        "42",
    ),
}

Actual output (note how instead of Some(String) we're getting Some(Some(String))

OptionalIntegerDiff {
    x: 0,
    ox: Some(
        42,
    ),
}
OptionalStringDiff {
    x: None,
    ox: Some(
        Some(
            "42",
        ),
    ),
}

Make it Option<Vec<String>> (or other compound type for that matter) and the end result is even more interesting.

benhall-7 commented 2 years ago

Hey, thanks for filing the issue! This one seems pretty interesting, in that it doesn't seem like there's necessarily a bug, but a better type definition for either or both of these types would help. In this case, the diff between None -> Some(T) is Some(T::Diff), but it's probably preferable to just let that be Some(T) with a cloned value. Although I need to consider the cases that yields when applying other diffs to this type