Skip to content

How can I prevent an Option field from being overwritten? #81

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Jackhr-arch opened this issue Mar 23, 2025 · 5 comments · Fixed by #82
Closed

How can I prevent an Option field from being overwritten? #81

Jackhr-arch opened this issue Mar 23, 2025 · 5 comments · Fixed by #82
Labels
enhancement New feature or request

Comments

@Jackhr-arch
Copy link

Here is the example:

#[derive(struct_patch::Patch, Debug, PartialEq)]
struct Foo {
    maybe_name: Option<String>,
}

let mut foo = Foo { maybe_name: None };

let mut patch = Foo::new_empty_patch();
patch.maybe_name = Some(Some("Should be kept".to_owned()));
foo.apply(patch);
assert_eq!(foo, Foo {
    maybe_name: Some("Should be kept".to_owned())
});

let mut patch = Foo::new_empty_patch();
patch.maybe_name = Some(Some("Being overwritten".to_owned()));
foo.apply(patch);
// The content is overwritten
assert_ne!(foo, Foo {
    maybe_name: Some("Should be kept".to_owned())
});

I hope there is something like

#[derive(struct_patch::Patch)]
struct Foo {
    #[patch(skip_if(Option::is_some))]
    maybe_name: Option<String>,
}
@yanganto
Copy link
Owner

Hi @Jackhr-arch,

You may misunderstood here.

#[derive(struct_patch::Patch, Debug, PartialEq)]
struct Foo {
    maybe_name: Option<String>,
}

let mut foo = Foo { maybe_name: None };

let mut patch = Foo::new_empty_patch();
patch.maybe_name = Some(Some("Should be kept".to_owned()));
foo.apply(patch);
assert_eq!(foo, Foo {
    maybe_name: Some("Should be kept".to_owned())
});

let mut patch = Foo::new_empty_patch();

// Current design does not fit your using case.
// In your example the field is some, so we need to patch the content of the field. 
// patch.maybe_name = Some(Some("Being overwritten".to_owned()));
// If the field `None`, there is nothing to patch
patch.maybe_name = None;

foo.apply(patch);
assert_eq!(foo, Foo {
    maybe_name: Some("Should be kept".to_owned())
});

The thing you want is only patch None not Some(_) of the Option<_> field in a instance. It is very Option field specific, so it should be #[patch(skip_some)]. Is there any possible to use #[patch(skip_if(fn))] in your case?

@Jackhr-arch
Copy link
Author

Jackhr-arch commented Mar 24, 2025

Is there any possible to use #[patch(skip_if(fn))] in your case?

No more in my case, but I think #[patch(skip_if(fn)] can support more options like Vec::is_empty so it might benefit.


In fact, I encounter some other problems.

#[derive(struct_patch::Patch, Debug, PartialEq)]
struct Foo {
    vec: Vec<u8>,
}

let mut foo = Foo { vec: vec![1, 2] };
let mut patch = Foo::new_empty_patch();
patch.vec = Some(vec![3, 4]);
foo.apply(patch);

// Not append
assert_eq!(foo, Foo { vec: vec![3, 4] })

Currently I solve this by using foo.into_patch() and #[patch(add = fn)], but I want some easier ways like #[patch(append = fn)], or #[patch(add_and_append = fn)]


In fact, if I use std::mem::take(&mut foo).into_patch() + patch and apply it to Foo::default(),

#[skip_some] and be replaced with fn keep<T>(a: Option<T>, b: Option<T>) -> Option<T> { a.or(b) },

while the other fields keep being overwritten with fn replace<T>(a: Option<T>, b: Option<T>) -> Option<T> { b.or(a) }

These makes things easier, what you think?

@yanganto
Copy link
Owner

yanganto commented Mar 24, 2025

Hmm, it is good to keep patch object the same, because the field of instance will be patched based on the field of Patch object is some or none. It is clear and straightforward

And we can provide another struct for your using case, I want to call it Filler. When we apply a filler is based on the the instance is empty or not.
This is straightforward with the DIY experience "filler to apply a wall with holes"
Follows are the syntax I de# quick. Are these looks good for you?

#[derive(struct_patch::Filler, Debug, PartialEq)]
struct Foo {
    maybe_name: Option<String>,
}

let mut foo = Foo { maybe_name: None };

let mut filler = Foo::new_empty_filler();
filler.maybe_name = Some("Should be kept".to_owned());
foo.apply(filler);
assert_eq!(foo, Foo {
    maybe_name: Some("Should be kept".to_owned())
});

let mut filler = Foo::new_empty_filler();

// The filler and not apply if the instance is filled
filler.maybe_name = Some("Being overwritten".to_owned());


foo.apply(filler);
assert_eq!(foo, Foo {
    maybe_name: Some("Should be kept".to_owned())
});

The using case for Vec, we can also define what is full or what is empty, so the filler can apply on the instance or not.

#[derive(struct_patch::Filler, Debug, PartialEq)]
struct Foo {
    #[filler(full=4)]
    vec: Vec<u8>,
}

let mut foo = Foo { vec: vec![1, 2] };
let mut filler = Foo::new_empty_filler();
filler.vec = vec![3, 4];
foo.apply(filler);

assert_eq!(foo, Foo { vec: vec![1, 2, 3, 4] })


let mut filler = Foo::new_empty_filler();
filler.vec = vec![5, 6];
foo.apply(filler);

// It is full, so the filler apply without effects.
assert_eq!(foo, Foo { vec: vec![1, 2, 3, 4] })

Following is the example to define empty, and we can not define empty and full at the same time, or there is also a default_is_empty, so we do not need empty=0.

#[derive(struct_patch::Filler, Debug, PartialEq, Default)]
struct Color {
    R: u8,
    G: u8,
    B: u8,
    #[filler(empty=0)]
    // or #[filler(default_is_empty)]
    alpha: u8,
}

let mut color = Color::default();
let mut filler = Coler::new_empty_filler();

//  Note: there is no R, G, or B in ColorFiler, because R, G, B are not `Option` or `Vec` field
// ColorFiler {
//   alpha: Option<u8>,
// }

filler.alpha = Some(50);
color.apply(filler);

assert_eq!(color, Color { R: 0, G: 0, B: 0, alpha: 50 })

If you think this is a good idea, we can work on this and release the Option part first, so you can use it in quick.

@yanganto yanganto added the enhancement New feature or request label Mar 24, 2025
@Jackhr-arch
Copy link
Author

Yeah I think that's a good idea.

I suggest a #[filler(extend = fn)] for some complex type such as Map<Key1, Map<Key2, Value>>

Vec, HashMap or BtreeMap they don't impl std::ops::Add but impl std::iter::Extend, so I use extend here.

#[filler(empty=0)], #[filler(full=4)] or #[filler(default_is_empty)] can be replaced with (or add) #[filler(skip_if = fn)] to support some other types that doesn't have a len fn.

And the behavior of applying FooFiller { vec: Some(vec![4, 5]) } to Foo { vec: vec![1, 2, 3] } where full = 4 requires some extra care


I've also got some ideas about this.

  • #[filler] is used to mark that this field is editable
  • #[filler(extendable)] for those who impl std::iter::Extend
  • #[filler(extend = fn)] for those who doesn't
  • #[filler(skip_if = fn)] to prevent field being overwritten
  • and for those have #[filler] without extend/extendable, just overwrite it

Maybe you can just work on current trait Patch rather creating a new one, since all I want is just a skip and extend?

@yanganto
Copy link
Owner

yanganto commented Mar 25, 2025

We are clear with the basic Option part, and we can make different issue focus on different topic. #83 #84 #85

In my view, the Filler is better use std::ops::Add, because the + op is easier to use, it is nothing about the extend or add existing in other struct or type. And we may not need #[filler(skip_if)], because the Filler currently is only implement Option.

The concept of Filler is to fill the empty space, so user need to define what is empty or what is full, then the Filler can work on that struct.

We can discuss each topic in issues, and it will be easier to move on. Thanks.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants