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 WithSliceDeepMerge option #234

Closed
wants to merge 1 commit into from

Conversation

sunsingerus
Copy link

PR for issue #233.
Introduce new option mergo.WithSliceDeepMerge which can be tuned with mergo.WithOverride or mergo.WithOverwriteWithEmptyValue.

Test cases are provided in issue233_test.go for mergo.WithSliceDeepMerge and combinations with mergo.WithOverride or mergo.WithOverwriteWithEmptyValue.

Test are being run successfully with go test.

@sunsingerus sunsingerus mentioned this pull request May 7, 2023
@sunsingerus sunsingerus force-pushed the WithSliceDeepMerge branch from 1094edd to 4c25c5b Compare May 7, 2023 18:22
@sunsingerus sunsingerus force-pushed the WithSliceDeepMerge branch from 4c25c5b to 7cab6cf Compare May 7, 2023 18:48
@sunsingerus sunsingerus force-pushed the WithSliceDeepMerge branch from 7cab6cf to fc6fffa Compare May 7, 2023 18:54
@sunsingerus
Copy link
Author

issues noted by codeclimate addressed

@sunsingerus
Copy link
Author

commits squashed for clarify

@sunsingerus
Copy link
Author

Hello @imdario, can you please suggest, what next steps should be taken in order to move on with this PR?

@darccio
Copy link
Owner

darccio commented May 18, 2023

@sunsingerus I'll review during the weekend.

@Slach
Copy link

Slach commented Jun 23, 2023

@imdario any news about this PR?

@darccio
Copy link
Owner

darccio commented Jun 25, 2023

@Slach I had no opportunity to review it in depth. I won't be able to try again until August as I'm getting married.

@darccio
Copy link
Owner

darccio commented Aug 24, 2023

Sorry but adding more complexity to that condition in deepMerge doesn't feel safe. I understand that tests are green, but I already accepted some PR in the past with all green tests, and it caused a broken version of Docker.

@darccio darccio closed this Aug 24, 2023
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.

3 participants