[WIP][confmap] Enable decoding nil values in confmap #11734
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
mahadzaryab1
force-pushed
the
enable-nil-decoding
branch
from
f4a9651
to
75840e6
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
| to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap())) | ||
| if !from.IsNil() { | ||
| to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap())) | ||
| } | ||
| } | ||
|
|
||
| return from.Interface(), nil |
There was a problem hiding this comment.
why are you not changing this to from? That's the main change that needs to be applied across all hooks, otherwise you never know under which condition they might return untyped nil and break the chain.
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11734 +/- ##
==========================================
- Coverage 91.61% 91.60% -0.01%
==========================================
Files 443 443
Lines 23770 23778 +8
==========================================
+ Hits 21776 21781 +5
- Misses 1620 1622 +2
- Partials 374 375 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -33,8 +33,7 @@ func TestLoadConf(t *testing.T) { | |||
| func TestToStringMapSanitizeEmptySlice(t *testing.T) { | |||
| cfg, err := LoadConf(filepath.Join("testdata", "empty-slice.yaml")) | |||
| require.NoError(t, err) | |||
| var nilSlice []interface{} | |||
| assert.Equal(t, map[string]any{"slice": nilSlice}, cfg.ToStringMap()) | |||
| assert.Equal(t, map[string]any{"slice": []interface{}{}}, cfg.ToStringMap()) | |||
There was a problem hiding this comment.
@yurishkuro What do you think of the change to this expectation here? It looks like it was conflicting with rest of the decoding flow. This is what empty-slice.yaml looks like:
slice: [] # empty slices are sanitized to nil in ToStringMapGiven that slice is explicitly identified, shouldn't slice be empty instead of nil?
There was a problem hiding this comment.
according to the comment in the fixture the current test is as expected. Does it not contradict the description of the hook?
There was a problem hiding this comment.
if there is indeed a contradiction, I would suggest opening a ticket with description, to let the maintainers opine what behavior they expect.
There was a problem hiding this comment.
PR is up at #11755 for the patch
mahadzaryab1
force-pushed
the
enable-nil-decoding
branch
from
a082c3b
to
4a58881
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
mahadzaryab1
force-pushed
the
enable-nil-decoding
branch
from
ac3972a
to
57ffb80
Compare
Description
Optionalunmarshaller in WIP do not merge - optional config fields #10260. This PR enables the decoding of nil values inconfmapto unblock the enablement of optional configs.Link to tracking issue
Towards ##10266
Testing
Documentation