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 DeepCopy capabilities for DynamicObj #35

Closed
kMutagene opened this issue Sep 23, 2024 · 9 comments
Closed

Add DeepCopy capabilities for DynamicObj #35

kMutagene opened this issue Sep 23, 2024 · 9 comments
Assignees

Comments

@kMutagene
Copy link
Member

kMutagene commented Sep 23, 2024

Update: renamed this issue so we can focus on the combine rework in another one: #43

Both should do the same (or there needs to be a _.combine instance method), and i think they should be adapted to not mutate any of the 2 objects but rather return a new object.

@github-actions github-actions bot added the Status: Needs Triage This item is up for investigation. label Sep 23, 2024
@kMutagene kMutagene moved this to Backlog in ARCStack Sep 23, 2024
@kMutagene kMutagene removed the Status: Needs Triage This item is up for investigation. label Sep 23, 2024
@kMutagene
Copy link
Member Author

Additionally, CopyDynamicPropertiesTo should not fail if overwrite is set to false. The expected behavior is that on overWrite = false, new properties are added to the target, but existing ones are kept as-is.

@kMutagene
Copy link
Member Author

for reference, this blocks nfdi4plants/ARCtrl#478

@kMutagene
Copy link
Member Author

This has been a gigantic rabbit hole, and the solution is not very satisfiying, but we at least have some Deep copy capabilities in #42 . It is i think impossible to return a completely copied result though, as we do not have enough knowledge about boxed dynamic props to deep copy any type/class (and there might be ones that cannot be deep copied anyways). I have settled on a "good enough" approach for now.

@HLWeil
Copy link
Member

HLWeil commented Dec 10, 2024

So typed objects without a "Copy" method will result in a "DynamicObject" with the same fields?

@kMutagene
Copy link
Member Author

So typed objects without a "Copy" method will result in a "DynamicObject" with the same fields?

Yes but Note that this is a draft PR. I think there is a point to be made that static properties should be copied as well. That way, we can "recover" the derived class if needed.

I think there is no other way for this method. This is the 'try maximum nested deep copy' method. I think as long as this is well documented everything is fine.

@HLWeil
Copy link
Member

HLWeil commented Dec 11, 2024

Oh, so currently only the dynamic properties are copied? Shouldn't it be a simple addition to copy the static fields too? E.g. decided by an optional boolean parameter?

Yes, there is no secure way to deep copy classes, even via Reflection. You can never be sure how the constructor is meant to be used.

@kMutagene
Copy link
Member Author

Shouldn't it be a simple addition

yes, but alas, this is a draft PR i'll let you know when it is ready to be reviewed ;)

@kMutagene
Copy link
Member Author

Note that this will not be closed via #42, combine still needs work afterwards

@kMutagene kMutagene changed the title Canonize CopyDynamicPropertiesTo and DynObj.combine Add DeepCopy capabilities for DynamicObj Dec 18, 2024
@kMutagene
Copy link
Member Author

Closed via #42

@github-project-automation github-project-automation bot moved this from In progress to Done in ARCStack Dec 18, 2024
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