-
Notifications
You must be signed in to change notification settings - Fork 25
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
Make sure path is not incorrectly modified via ref #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this!
Could you also fix the failing test? Looks like we're testing for the incorrect behavior.
https://github.com/d4l3k/messagediff/blob/master/example/atom_test.go#L16
d.Modified[&path] = bVal.Interface() | ||
localPath := make(Path, len(path)) | ||
copy(localPath, path) | ||
d.Modified[&localPath] = bVal.Interface() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of just copying in that one specific location, would be good to move it to the top of the diff
function. That way it fixes all cases of that.
localPath := make(Path, len(path))
copy(localPath, path)
path = localPath
Near https://github.com/d4l3k/messagediff/pull/8/files#diff-2aea0ca6be2f84668b7aefadf8f8ac26R44.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup makes sense. Will fixup.
Tests updated and feedback addressed :) |
Thanks for fixing that! |
Sometimes, the path name would be incorrectly listed as the last path processed. This makes sure the path is not updated via the
path
pointer.