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

fixed issue with not overriding of existing value of map #8

Merged
merged 2 commits into from
Apr 6, 2015

Conversation

nl5887
Copy link
Contributor

@nl5887 nl5887 commented Jan 29, 2015

fixed issue with not overriding of existing value of map, and added expect

@darccio
Copy link
Owner

darccio commented Jan 29, 2015

By "overriding existing value" I assume you mean same key in both maps but different values. I need to check this pull before accepting it, although it looks fine.

But Mergo doesn't override values in destination by design, so I'm not sure if this pull shouldn't have some more changes to keep current semantics. We may add a parameter or create a new function called "Crush" "Fusion" or "OverridingMerge" (just some quick ideas).

@nl5887
Copy link
Contributor Author

nl5887 commented Jan 29, 2015

In my case the use case is that I got a default configuration (dest) and a yaml configuration (src), default values of the configuration will be overridden by configuration.

What about MapWithOverwrite and MergeWithOverwrite? Have to do a lot of testing (and add some extra tests), but this will work. Have a first version now, but old tests are (still) failing, will work on that.

@darccio
Copy link
Owner

darccio commented Jan 29, 2015

Fine for me. Anyway, I would use Mergo in the opposite way, using your yaml configuration as destination and the default one as source. In this way, you get the same result, unset options will have your default values while keeping your custom values.

@nl5887
Copy link
Contributor Author

nl5887 commented Feb 2, 2015

I tested that also, but for me it wasn't updating or adding values if the destination (map in this case) already exists. The patch attached will introduce 2 new functions, MergeWithOverwrite and MapWithOverwrite, which will only overwrite with non empty values.

darccio added a commit that referenced this pull request Apr 6, 2015
fixed issue with not overriding of existing value of map
@darccio darccio merged commit 2f8eb1d into darccio:master Apr 6, 2015
@darccio darccio mentioned this pull request Apr 6, 2015
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