return nil instead of empty map #13
Conversation
|
| @@ -87,7 +87,7 @@ func (d Directives) Int(key string) (int, bool) { | |||
| func (d Directives) Object(key string) Directives { | |||
| v, ok := d[key] | |||
| if !ok { | |||
| return make(Directives) | |||
| return nil | |||
| } | |||
| m, ok := v.(map[string]interface{}) | |||
| if !ok { | |||
There was a problem hiding this comment.
The following line should also be nil, right?
There was a problem hiding this comment.
if it exists but theres nothing in the map, then we should likely keep it? I pushed this already but can make changes if we need
There was a problem hiding this comment.
the other question is does anything outside of env vars use this today?
There was a problem hiding this comment.
This if !ok on the type assertion would mean that e.g. environment_variable = 10 - that it's not a table. IMO it should be treated the same as if it weren't set.
There was a problem hiding this comment.
yeah that maybe we would want to error and say that its set to an incorrect type even, should i add a card for this? I dont think weve really touched the logic of directives too much. Were unblocked for now but youre right we may want to add more to this
| keyType, pResult := p.determinePersistType(f, annot) | ||
| if pResult == nil { | ||
| if annot.Capability.Directives.Object(core.EnvironmentVariableDirective) != nil { |
There was a problem hiding this comment.
| if annot.Capability.Directives.Object(core.EnvironmentVariableDirective) != nil { | |
| if len(annot.Capability.Directives.Object(core.EnvironmentVariableDirective)) == 0 { |
This will work for both nil and empty.
There was a problem hiding this comment.
it doesnt let you do len on that object
There was a problem hiding this comment.
Ah, yeah you'd have to len(map[string]interface{}(..)) maybe worth adding a method to Directive at some point (receivers can be nil).
• Does any part of it require special attention?
• Does it relate to or fix any issue?
fixes the bug
Standard checks