feature: control merging behavior #66
Conversation
|
Looks like there are merge conflicts on this after the #64 merge. |
|
Should be good now. |
|
Hi @grount, |
|
@knadh what comments? |
|
Ah, are you unable to see the inline comments I added to the PR? https://github.com/knadh/koanf/pull/66/files
|
|
As you can see they are pending, you need to submit your comments in the top right (I think). |
|
What I meant was, would you be able to make the fixes posted in the inline comments so that we can merge this PR? |
koanf.go
Outdated
| @@ -55,10 +69,23 @@ type UnmarshalConf struct { | |||
| // or a / for `parent/child/key`. | |||
| func New(delim string) *Koanf { | |||
| return &Koanf{ | |||
There was a problem hiding this comment.
Can just return this:
return NewWithConf(Conf{Delim: delim})| maps.IntfaceKeysToStrings(c) | ||
| maps.Merge(c, ko.confMap) | ||
| if ko.conf.StrictMerge { |
maps/maps.go
Outdated
| @@ -132,6 +145,51 @@ func Merge(a, b map[string]interface{}) { | |||
| } | |||
| } | |||
|
|
|||
| func MergeStrict(a, b map[string]interface{}) error { | |||
| mergeError := MergeStrictError{Errors: []error{}} | |||
There was a problem hiding this comment.
Not sure if skipping failures and collecting all errors is beneficial. We can just keep this idiomatic and return on the first error. That'll also combine MergeStrict() and mergeStrict().
There was a problem hiding this comment.
Assuming I have 10 errors, that would require 10 runs to figure out all the errors.
That way he can fix all the possible errors.
WDYT?
There was a problem hiding this comment.
That could be said of any function that does multiple things, but collecting errors like that is an uncommon pattern. For instance, a JSON payload passed to json.Unmarshal() could have an indefinite number of errors, but the execution halts at the first error which is then immediately returned. We should keep this simple and return at the first error.
| // Does the key exist in the target map? | ||
| // If no, add it and move on. | ||
| bVal, ok := b[key] | ||
| if !ok { |
There was a problem hiding this comment.
Can we keep mergeStrict() exactly like merge() except for the additional type check that'll be added here?
There was a problem hiding this comment.
I still need the fullKey to return the error message with the path to the key, otherwise it would be misleading.
| @@ -132,6 +145,51 @@ func Merge(a, b map[string]interface{}) { | |||
| } | |||
| } | |||
|
|
|||
| func MergeStrict(a, b map[string]interface{}) error { | |||
|
@grount apologies. I added these comments weeks ago but missed hitting "Submit" to make them visible. Confusing GitHub UX. |
Signed-off-by: Daniel Gront <grount1@gmail.com>
|
@knadh ping |
issue #62