Skip to content

Deep copy improvements#46

Merged
kMutagene merged 7 commits intomainfrom
deepCopy
Jan 27, 2025
Merged

Deep copy improvements#46
kMutagene merged 7 commits intomainfrom
deepCopy

Conversation

@kMutagene
Copy link
Copy Markdown
Member

@kMutagene kMutagene commented Jan 3, 2025 •
edited
Loading

This PR will

  • introduce more type safe deep copy capabilities for .NET:
    • ResizeArrays and Dictionaries containing any combination of basic F# types
    • Dictionaries containing DynamicObj as keys or values in any combination with DynamicObj or basic F# types as keys or values
  • Keep js/py results consistent

This was linked to issues Jan 3, 2025
Add a flag for DeepCopyPropertiesTo that enabled to omit static props #44
Closed
Expand DeepCopy logic to mutable collections that contain primitives #45
Closed
Deep copy: add native fallbacks for objects in js/py #47
Closed
@kMutagene
Copy link
Copy Markdown
Member Author

kMutagene commented Jan 24, 2025

@HLWeil

I would say this is ready for review. While i added ~300 tests in this PR, there are lots of test still to be added. However, to unblock downstream work i think this should be ready for at least a beta release, so let's get this merged and add more tests later

@kMutagene kMutagene marked this pull request as ready for review January 24, 2025 14:17
@kMutagene kMutagene requested a review from HLWeil January 24, 2025 14:18
@kMutagene kMutagene mentioned this pull request Jan 24, 2025
Open
6 tasks
@kMutagene
Copy link
Copy Markdown
Member Author

kMutagene commented Jan 24, 2025

for future reference, here is what is missing: #48

HLWeil
HLWeil reviewed Jan 24, 2025
View reviewed changes
Copy link
Copy Markdown
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.

That's some commitment 💯

Is there any test case for a nested DynamicObj? If not, I think it would be a good mind-easer to have at least 3 very basic tests for this scenario:

  • DynamicObj with Single field (e.g. string) in the DynamicObj to be copied
  • DynamicObj with Single field (e.g. string) in ResizeArray in the DynamicObj to be copied
  • DynamicObj with Single field (e.g. string) in Dictionary in the DynamicObj to be copied

Comment thread tests/DynamicObject.Tests/CopyUtils.tryDeepCopyObj/DynamicObj.fs
Copy link
Copy Markdown
Member

@HLWeil HLWeil Jan 24, 2025

Choose a reason for hiding this comment

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

These are the missing tests you referenced in #48?

Copy link
Copy Markdown
Member Author

@kMutagene kMutagene Jan 27, 2025

Choose a reason for hiding this comment

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

https://github.com/CSBiology/DynamicObj/blob/main/tests/DynamicObject.Tests/DynamicObj/DeepCopyProperties.fs Contains a few tests for DynamicObj.DeepCopyProperties that covers nested Dynamic Objects. I was mainly referring to the underlying function CopyUtils.tryDeepCopyObj still missing tests

@kMutagene kMutagene merged commit c77be4c into main Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HLWeil HLWeil HLWeil approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

2 participants

@kMutagene @HLWeil