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

Incorrect handling of sub-collections #7

Closed
stiiifff opened this issue May 19, 2016 · 3 comments
Closed

Incorrect handling of sub-collections #7

stiiifff opened this issue May 19, 2016 · 3 comments

Comments

@stiiifff
Copy link

Hi Khalid,

First of all, thanks for the very useful library, I'm using it in a client project. I haven't found another implementation that is as handy and low-ceremony as yours 👍
I just found a little issue in sub-collections, let me explain it.
If you have an original object:
{ "Children": [ { "Name": "Alice" } ] }
and a modified one:
{ "Children": [ { "Name": "Alice" }, { "Name": "Bob" } ] }
If I do a GenerateDiff and then PatchObject one the original object, I get this:
{ "Children": [ {"Name": "Alice"}, null ] }

I have addressed the issue by making 2 small changes in the Patch method:
'return array' in the block that deals with arrays, 'return sourceObj' in the block that deals with objects, and remove the 'return sourceJson' at the end of the method. Problem is then fixed, and I think the code is a bit less confusing as well.

Attached is a small unit test that highlights the issue.
Would be awesome if you could integrate this fix, thx :)

ObjectDiffPatchArrayIssue.zip

@khalidsalomao
Copy link
Owner

@stiiifff ,

Thanks for taking time to address the issue and prepare a fix with tests included! 😄

Could you prepare a Pull request ? It would make the code review and integration a lot easier for me!

It is very simple:
1- create a fork
2- commit your changes
3- use github button New pull request

Thanks!
Best!

@khalidsalomao
Copy link
Owner

@stiiifff I just updated Nuget.org with a version that should address this problem. Could you check it out?
I applied a patch that @kkostov submitted 5 months ago.
Best!

@stiiifff
Copy link
Author

stiiifff commented May 22, 2016

Thanks for taking the time, I will check it out. Didn't have time to submit a PR but next time I'll do ;)

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