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 Deep/Shallow Copy methods #42

Merged
merged 6 commits into from
Dec 17, 2024
Merged

Add Deep/Shallow Copy methods #42

merged 6 commits into from
Dec 17, 2024

Conversation

kMutagene
Copy link
Member

@kMutagene kMutagene commented Dec 9, 2024

This PR

  • Adds distinct methods for:
    • ShallowCopyDynamicProperties(To): The 'old' copy method, does not atttempt to do any copy logic on nested dynamic objects, lists, etc. Also, copied properties are reference equal where applicable (meaning mutating them on the source object will mutate them on the copied object). It is now properly named and has better xml docs
    • DeepCopyDynamicProperties(To): Attempts to
      • recursively copy dynmic properties
      • create new instances of types where possible, e.g. for DynamicObj (collections) or classes that implement ICloneable
      • Has some implications on derived types from DynamicObj, e.g. them loosing their superclass by being cast to DynamicObj.

@kMutagene kMutagene requested a review from HLWeil December 17, 2024 09:45
@kMutagene kMutagene marked this pull request as ready for review December 17, 2024 09:45
@kMutagene
Copy link
Member Author

hopefully only tests left to add

@kMutagene
Copy link
Member Author

kMutagene commented Dec 17, 2024

Decided to leave further changes to combine up for a new PR to move with other things, combine does a deep copy first, which should already mitigate some mutation issues. combine is another beast, just doing a new pr soon

@kMutagene kMutagene changed the title Add Deep/Shallow Copy methods, improve Combine Add Deep/Shallow Copy methods Dec 17, 2024
Copy link
Member

@HLWeil HLWeil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Only two minor requests

target

// internal helper function to deep copy a boxed object (if possible)
static member internal tryDeepCopyObj (o:obj) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefere if you could move this into a publicly accessible helper module (RequireQualifiedAccess?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so specific, i am not sure if it is ever useful for other purposes. Also, this function uses the DynamicObj class and is used in it

@@ -4,7 +4,7 @@ open Fable.Pyxpecto

let all = testSequenced <| testList "DynamicObj" [
ReflectionUtils.Tests.main
DynamicObj.Tests.main
DynamicObjs.Tests.main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the "s"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was the name of the file i refactored into separate ones 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah also, this:

A namespace and a module named 'DynamicObj.Tests' both occur in two parts of this assembly

@HLWeil HLWeil merged commit b02c6fc into main Dec 17, 2024
2 checks passed
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