feat: Implement patch for Box and Option

[taorepoara]

In order to ease the use of standard types like Box and Option I created default implementation of the Patch trait for them.

Each type as is own feature:

• Box: box
• Option: option

I also add a from_patch attribute to implement From of the generated patch struct for the main struct.

[yanganto]

The into transition from patch to instance will confuse some scenarios.
The real behavior is patch_on_default but not into

If the default is not empty, it will confuse, for example:

/// In s.rs, S is defined 
#[derive(Patch)]
struct S {
  a: usize,
  b: usize,
}

impl Default for S {
  fn default() -> Self {
    S {
      a: 100,
      b: 0,
    }
  }
}

---

/// In another file, S is used
use s::*;

let patch = SPatch {
  a: None,
  b: Some(2),
}

let patched_s = patch.into();
/// It is not straightforward, 
/// especially in big projects with a lot of developers
///  S { a:100, b: 2, }

I avoided implementing From trait in this crate because it will easily be misleading. Besides, the from feature will need Default trait, and from is in the default features of this crate, so it will reduce the usability of this crate.

Also, I think the patch_on_default is still misleading, the following way is good enough and easier to read for another developer.

let patched_s = S::default().apply(patch);

Is it possible to remove the from feature?
What do you think? I believe less is more. Syntax sugar should fit the needs exactly and save time.

[taorepoara]

The From is necessary for the Option implementation.

IMO to create a main struct from his patch, the From trait this the best solution.

and from is in the default features of this crate, so it will reduce the usability of this crate

from is not in the default feature and event more it's only enabled by a specific attribute. The From can be manually implemented.

I can move the from feature in another PR if you prefer but it is pretty useful when using Option.
I also can rename the feature from_default or from_patch.

Let me know 😃

[taorepoara]

By the way, I use this implementations in one repo of mine: https://github.com/lenra-io/dofigen/blob/180-extends-files/src/dofigen_struct.rs

It might be more explicit with an example

[yanganto]

The From is necessary for the Option implementation.

No, it is not necessary, you can do it by default like you do in from

fn from(patch: #name #generics) -> Self {
    let mut s = Self::default();
    s.apply(patch);
    s
}

First, it is better to let others know this is from default and then apply the patch.
Second, the option feature only requires the Default impl not From impl.
Less dependency is better, and please doc on this, such that others can easily know what they need to implement Default to use option.

---

I can move the from feature in another PR if you prefer but it is pretty useful when using Option. I also can rename the feature from_default or from_patch.
We can split and merge std without from.

Renaming the feature does not help, I still think .into() is not a good thing here.
The behavior is patched on the default instance, and the following is still one line with just several chars longer.

let patched_s = S::default().apply(patch);  // No `from` trait
let patched_s: S = patch.into();            // With `from` trait

It is not straightforward to use into() here for me.
If you insist it is straightforward, we can use another PR on From trait to think about this.

---

By the way, why do people using the option feature tend to use the box feature at the same time?
Could we drop std feature? Not only it is unclear on the feature name, but also option and box features are not strongly related to be a group feature.

[taorepoara]

First, it is better to let others know this is from default and then apply the patch.
Second, the option feature only requires the Default impl not From impl.
Less dependency is better, and please doc on this, such that others can easily know what they need to implement Default to use option.

The Default and From traits are both in the Rust core lib, so I don't think that it changes the number of depedencies, isn't it ? They are both managed by the std.

If it's not, I think it's cleaner to keep From instead of Default.
Using From instead of Default let the user define a custom way to transform the patch into the original struct.
In my understanding of the Rust language, this is the purpose of the From/Into.
Using the Default is a workaround that works only with this lib.

Keeping both of them would let the dev have different behaviors for the two of them.

If you insist it is straightforward, we can use another PR on From trait to think about this

Ok, I'll move this in another PR.

Less dependency is better, and please doc on this, such that others can easily know what they need to implement Default to use option.

Here is the compile error when it's not satisfied: the trait bound 'dofigen_struct::Stage: From<StagePatch>' is not satisfied

But I'll add some docs, it's always better to add docs, sorry I often forget ^^

By the way, why do people using the option feature tend to use the box feature at the same time?
Could we drop std feature? Not only it is unclear on the feature name, but also option and box features are not strongly related to be a group feature.

I created the std feature to enable all the features linked to the std lib. For now there is only box and option, but I'm thinking of implementing Patch for other std types like HashMap and other since they only can be implemented into your repo.
I can remove it if you prefer adding them one by one.

[taorepoara]

By the way, this does not work since the apply function does not return value: let patched_s = S::default().apply(patch);

[yanganto]

By the way, this does not work since the apply function does not return value: let patched_s = S::default().apply(patch);

Okay, my bad, I did not carefully review the previous PR, so I forgot the interface changed.
It may be the time to implement Add

impl ops::Add<SPatch> for S {
    type Output = S;

    fn add(self, _rhs: SPatch) -> S {
       ...
    }
}

Such that we can have

let patched_s = S::default() + patch;

It will be clear and short in one line.

[taorepoara]

It may be the time to implement Add

Nice !

I'll try to implement it. I'll do it in another feature, you'll tell me if we do it as default or not 😃

[yanganto]

It may be the time to implement Add

Nice !

I'll try to implement it. I'll do it in another feature, you'll tell me if we do it as default or not 😃

We can make it the default, and we implement both ends of Add, namely we also need this.

impl ops::Add<S> for SPatch {
    type Output = S;

    fn add(self, _rhs: S) -> S {
       ...
    }
}

If the Add is good, do you like this?

impl<T, P> Patch<Option<P>> for Option<T>
where
    T: Patch<P> + Default,
{
    fn apply(&mut self, patch: Option<P>) {
        if let Some(patch) = patch {
            if let Some(self_) = self {
                self_.apply(patch);
            } else {
                *self = Some(T::default() + patch);
            }
        } else {
            *self = None;
        }
    }
}

And we need #[cfg(feature = "option")] condition for impl ops::Add<P> for T but not impl ops::Add<P> for Option<T>.

Let option be dependent on Default not From, so the feature works much independently because the implementation of Default for T is easier than Form for a user because the T is the original type defined by the user.
Besides, the user does not need to implement something around these types inside the macro if the user uses option only.

This also fits your use case.

I know this implementation will be harder than you originally proposed, but I still try to avoid using .into() inside of the macro, it will be nice if others possibly want to join the project. 🙏

If this is good, could we let Add feature go in before the Box and Option, because the overall implementations in the repo will be much easier to read and let people know how it works.
Please let me know if this looks good from your end. 😄

[taorepoara]

I know this implementation will be harder than you originally proposed, but I still try to avoid using .into() inside of the macro, it will be nice if others possibly want to join the project. 🙏

It's not harder to implement with Default, but IMO the use of From instead of Default is still the best solution since Default is for another purpose.

Using From for Option does not need to use into() inside the macro, and there is no into() in the current version of the macro (there were in my first implementation before understanding that a problem was coming from somewhere else).

I'll squash the commits to make it cleaner.

If this is good, could we let Add feature go in before the Box and Option, because the overall implementations in the repo will be much easier to read and let people know how it works.

Ok, I'll create a specific PR for that 😃

[yanganto]

Sorry, I think twice.
If we patch on a None, should we get None here or an instance from the patch? Like the following PR, and it seems good with the tests.
https://github.com/taorepoara/struct-patch/pull/1

However, I agree this is better with from than applying a patch on default, and we do not need another PR ahead.

[taorepoara]

If we patch on a None, should we get None here or an instance from the patch? Like the following PR, and it seems good with the tests.

IMO the better solution is to have an instance from the patch and that's why we need the From.

Example

Here is a basic use case that might be clearer.

struct User {
  name: String,
  address: Option<Address>,
}

struct Address {
  street: Option<String>,
  country: String,
}

If we have an initial user like this:

let user = User {
  name: "Thomas",
  address: None,
}

If we want to patch the user to set his address (fully or partially) with the next patch:

let patch = UserPatch {
  address: Some(Some(AddressPatch {
    country: Some("France"),
    street: None,
  }))
}

We need to define it from the None value in the main struct.
Here is the patch result this way:

User {
  name: "Thomas",
  address: Some(Address {
    street: None,
    country: "France",
  }),
}

If we don't, the address won't ever be defined from a patch.

Hope it's clearer.

[yanganto]

Hi,
Please give an example under examples.
Let will be 100% clear, and good for other's with the same use case.

[yanganto]

Hi @taorepoara ,
You can use git add -f to add the example. The .gitignore about examples helps you to test the example in different branches. Sorry for the inconvenience.

[yanganto]

Can we do this in the test?
Such that we do not need a lot of #[cfg(feature)] and easier to read.

#[cfg(feature = "option")]
fn main() {
...
}
#[cfg(not(feature = "option"))]
fn main() { }

And put the #[cfg(feature = "option")] on the struct, not fields, if these are not inside the main.

Could you remove the option example in the no default feature set of CI?
We have nothing meaningful to test with this condition, but it still takes time in CI.