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

merge struct fields of type interface{} that contain data with the same concrete type #147

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

bionoren
Copy link

If a struct has two fields that are of interface type but contain data that is the same type, we should be able to deepMerge into that. This patch adds test coverage and support for this use case.

What this does, really, is

  1. move the dst.Kind() != ptr && isAssignable check to the end (because it took precedence over the "else deepMerge" case)
  2. only call deepMerge if the underlying Elem() values are the same kind
  3. modify the deepMerge call so that dst is populated (otherwise, if the value being passed to deepMerge has "CanSet() == false", which it does, then the result is lost).

…a that is the same type, we should be able to deepMerge into that. This patch adds test coverage and support for this use case
@darccio darccio self-requested a review July 16, 2020 18:03
@darccio darccio merged commit a0c19e4 into darccio:master Jul 16, 2020
darccio added a commit that referenced this pull request Jul 17, 2020
@darccio
Copy link
Owner

darccio commented Jul 17, 2020

"Reopening" this as PR #105 introduced too many bugs, and this issue isn't fixed with the current state of master.

@darccio darccio mentioned this pull request Jul 17, 2020
@darccio
Copy link
Owner

darccio commented Jul 18, 2020

@bionoren Sorry for the inconvenience, but I'm not able to apply your solution to the current implementation (it is not longer mergeable as PR #105 modified too many lines). If you can open a new PR, I'll be happy to merge as soon as it is ready.

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