-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Shl
between instance and patch
#32
Conversation
Co-authored-by: Thomas DA ROCHA <thomas.darocha@lenra.io>
Hi @taorepoara, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a Add between two patches would be nice two !
* feat: Implement Add<patch> for patch
Just wonder will pub trait Shl<Rhs = Self> {
type [Output](https://doc.rust-lang.org/std/ops/trait.Shl.html#associatedtype.Output);
// Required method
fn [shl](https://doc.rust-lang.org/std/ops/trait.Shl.html#tymethod.shl)(self, rhs: Rhs) -> Self::[Output](https://doc.rust-lang.org/std/ops/trait.Shl.html#associatedtype.Output);
} And it will be clear for the user, that the right-hand side always patches on the left-hand side, which can be a patch or an instance. |
I think that add is better but maybe we should only manage main struc as left operand |
That is what I think,
It will be straightforward, and I think we do not have a feature for this anymore. Let me implement with an example at the same time. |
Add
between instance and patchShl
between instance and patch
We need to check the type inside the macro, else the The behaviors are not the same, it is replacing without renaming, but merging with renaming |
Yes it won't work with std types since we can't implement Add for them (or maybe we can ?). A solution for those cases would be to let the possibility to define a function in Patch(add = "...") attribute on fields that overloads the behavior.
IMO this is the expected behavior, since we want to combine patches, so only there patchable fields. |
Would it be good for a parameter called At first, I feel I agree we can let the happy case go first with this (Some(a), Some(b)) => panic!("There are conflict patches in {}", stringy(field)). such that user can do let new_instance = orig_instance << patch_1 << patch_2; or let overall_patch = patch_1 + patch_2; // the user needs to make sure there is no conflict field
let new_instance = orig_instance << overall_patch; Do you agree we do this first in this PR, and then, we can discuss which one will be good from |
Sounds good to me. By the way, what I meant with the fn my_add_fx(a: SomeThingPatch, b: SomeThingPatch) {
...
}
#[derive(Patch)]
struct OtherThing {
#[patch(add = "my_add_fx"]
some_thing: SomeThing,
} |
Great, it is complete and will be flexible and extendable for users. |
Hi @taorepoara,
Could you help to review on this?