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

Recursive patch merging #42

Closed
taorepoara opened this issue Aug 14, 2024 · 4 comments
Closed

Recursive patch merging #42

taorepoara opened this issue Aug 14, 2024 · 4 comments

Comments

@taorepoara
Copy link
Contributor

When using the << operator between two patches, it merges them together.

For now if both patches have the same field defined, we use the right end value.
It would be better IMO to merge both values if they are patches.

This lead to a problem: Option.

Merging Option patches is not implemented by default and can't be implemented.

I see 2 solutions to fix that:

  1. Change the implementation of Patch<Option<P>> for Option<T> where T: Patch<P> to use a custom struct: OptionPatch. The best choice IMO since we can implement easily other traits on this new struct.
  2. Adapt the derive to always implement Shl<Option<$patch>> for Option<$patch> which could lead to many unuseful implementations

@yanganto What do you think ?

@yanganto
Copy link
Owner

#43 modify the option example with <<, and everything looks good.

Can you please modify the example for me?
Such that it will see a use case we can not fulfill.

The example is also the spec when we need to implement something, the way is like TDD, we can make sure what we will do always fits the user's need. The example is not in vain.

@yanganto
Copy link
Owner

yanganto commented Aug 14, 2024

Thanks for the clear example about the bug. ❤️

I have another solution for this.

  1. just remove shl for patch, so the user can not patch on a patch.

What do you think about it?

@yanganto
Copy link
Owner

If you want to solve this, we can keep it. The implementation is up to you. For me, 3 is good. 😄

@yanganto
Copy link
Owner

fixed by #45

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

No branches or pull requests

2 participants