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

Add Shl between instance and patch #32

Merged
merged 15 commits into from
Aug 12, 2024
Merged

Add Shl between instance and patch #32

merged 15 commits into from
Aug 12, 2024

Conversation

yanganto
Copy link
Owner

@yanganto yanganto commented Aug 9, 2024

Hi @taorepoara,

Could you help to review on this?

struct-patch-derive/src/lib.rs Outdated Show resolved Hide resolved
struct-patch-derive/src/lib.rs Outdated Show resolved Hide resolved
struct-patch-derive/src/lib.rs Outdated Show resolved Hide resolved
@yanganto
Copy link
Owner Author

yanganto commented Aug 9, 2024

Hi @taorepoara,
Would you might to take a look again? 😄
Thanks in advanced. 🙏

Copy link
Contributor

@taorepoara taorepoara left a 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 !

struct-patch-derive/src/lib.rs Outdated Show resolved Hide resolved
* feat: Implement Add<patch> for patch
@yanganto
Copy link
Owner Author

Just wonder will << be a better operator than +

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.

@taorepoara
Copy link
Contributor

I think that add is better but maybe we should only manage main struc as left operand

@yanganto
Copy link
Owner Author

I think that add is better but maybe we should only manage main struc as left operand

That is what I think,

  • We remove instance + patch, but provide instance << patch
  • We can keep patch + patch with conflict patch field (Some(a), Some(b)) => Some(a + b).
  • When doing patch << patch, it will be (Some(a), Some(b)) => Some(b)

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.

@yanganto yanganto changed the title Add Add between instance and patch Add Shl between instance and patch Aug 12, 2024
@yanganto
Copy link
Owner Author

yanganto commented Aug 12, 2024

We need to check the type inside the macro, else the + operator does not always work here.
https://github.com/taorepoara/struct-patch/blob/8e0b86f6d7a17e8e3846589124f19a220afb9f32/struct-patch-derive/src/lib.rs#L131

The behaviors are not the same, it is replacing without renaming, but merging with renaming
https://github.com/taorepoara/struct-patch/blob/8e0b86f6d7a17e8e3846589124f19a220afb9f32/struct-patch-derive/src/lib.rs#L138

@taorepoara
Copy link
Contributor

taorepoara commented Aug 12, 2024

We need to check the type inside the macro, else the + operator does not always work here.

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.

The behaviors are not the same, it is replacing without renaming, but merging with renaming

IMO this is the expected behavior, since we want to combine patches, so only there patchable fields.
The field add attribute could be defined on non patchable fields two to overload that behavior.

@yanganto
Copy link
Owner Author

yanganto commented Aug 12, 2024

Would it be good for a parameter called Patch(addable_fields=...) of the struct or #[patch(addable)] of the field?

At first, I feel let s = S::default() + patch will be a short and clear syntax in this comment.

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 Patch(add = "..."), Patch(addable_fields = "..."), and #[patch(addable)] in the next PR?

@taorepoara
Copy link
Contributor

taorepoara commented Aug 12, 2024

Do you agree we do this first in this PR, and then, we can discuss which one will be good from Patch(add = "..."), Patch(addable_fields = "..."), and #[patch(addable)] in the next PR?

Sounds good to me.

By the way, what I meant with the patch(add = "...") was to let the dev define function to handle the add:

fn my_add_fx(a: SomeThingPatch, b: SomeThingPatch) {
  ...
}

#[derive(Patch)]
struct OtherThing {
  #[patch(add = "my_add_fx"]
  some_thing: SomeThing,
}

@yanganto
Copy link
Owner Author

Great, it is complete and will be flexible and extendable for users.
Thanks for your help. I will merge this after #31, and fix the possible conflict in readme. If you still find out something please let me know in the PR. 😄

@yanganto yanganto mentioned this pull request Aug 12, 2024
2 tasks
@yanganto yanganto merged commit 0ee5705 into main Aug 12, 2024
3 checks passed
@yanganto yanganto deleted the add branch August 12, 2024 23:13
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

Successfully merging this pull request may close these issues.

2 participants