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 new WithSliceDeepMerge config option #180

Closed
wants to merge 2 commits into from
Closed

Add new WithSliceDeepMerge config option #180

wants to merge 2 commits into from

Conversation

arnaudbriche
Copy link
Contributor

No description provided.

} else {
if err = deepMerge(dstElem, srcElem, visited, depth+1, config); err != nil {
return
} else {
Copy link

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block

} else {
if err = deepMerge(dstElem, srcElem, visited, depth+1, config); err != nil {
return
} else {
Copy link

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block

@sourcelevel-bot
Copy link

Hello, @arnaudbriche! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@sourcelevel-bot
Copy link

SourceLevel has finished reviewing this Pull Request and has found:

  • 2 possible new issues (including those that may have been commented here).

See more details about this review.

@@ -231,6 +266,39 @@ func deepMerge(dst, src reflect.Value, visited map[uintptr]*visit, depth int, co
return
}
}
} else if sliceDeepMerge {
Copy link

Choose a reason for hiding this comment

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

Avoid deeply nested control flow statements.

@darccio
Copy link
Owner

darccio commented May 25, 2022

@arnaudbriche Check if #210 fixes your use case.

@darccio darccio closed this May 25, 2022
drenizg added a commit to everisopennetworks/mergo that referenced this pull request Jul 29, 2022
@drenizg
Copy link

drenizg commented Aug 2, 2022

Hi!

@arnaudbriche Check if #210 fixes your use case.

I tested this and didn't work as expected for the proposed feature. I also tested the code in this PR and were able to make it work for my use case.

Would it be possible to reopen this PR? Perhaps another PR should be opened? If needed, I can work on whatever was blocking this PR to be merged.

@darccio
Copy link
Owner

darccio commented Aug 2, 2022

@drenizg Sure, don't hesitate to work on this. Thanks! I think you'll need to reopen a new PR, as this one is not possible. The original repo was closed.

drenizg added a commit to everisopennetworks/mergo that referenced this pull request Aug 25, 2022
Added validations to some reflection calls that were causing panic: "reflect: call of reflect.Value.IsZero on zero Value"
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.

None yet

3 participants