Skip to content

Add call to InitDefaults when structs implement Initializer interface during Unpack.#141

Merged
blakerouse merged 11 commits intoelastic:masterfrom
blakerouse:init-defaults
Jan 24, 2020
Merged

Add call to InitDefaults when structs implement Initializer interface during Unpack.#141
blakerouse merged 11 commits intoelastic:masterfrom
blakerouse:init-defaults

Conversation

@blakerouse
Copy link
Copy Markdown
Contributor

@blakerouse blakerouse commented Jan 21, 2020

Call the InitDefaults during Unpack when a structure implements the Initializer interface. This allows a structure to define its default values without having to push the defaults into the loaded configuration.

Closes #104

@blakerouse blakerouse requested a review from urso January 21, 2020 19:20
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 21, 2020

Codecov Report

Merging #141 into master will decrease coverage by 0.34%.
The diff coverage is 82.05%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #141      +/-   ##
=========================================
- Coverage   77.94%   77.6%   -0.35%     
=========================================
  Files          23      24       +1     
  Lines        2612    2643      +31     
=========================================
+ Hits         2036    2051      +15     
- Misses        425     440      +15     
- Partials      151     152       +1
Impacted Files Coverage Δ
ucfg.go 90% <ø> (ø) ⬆️
unpack.go 85.39% <100%> (+0.33%) ⬆️
reify.go 69.83% <100%> (+1.19%) ⬆️
initializer.go 63.15% <63.15%> (ø)
merge.go 80% <0%> (-2.15%) ⬇️
path.go 83.55% <0%> (-1.98%) ⬇️
types.go 73.31% <0%> (-0.93%) ⬇️
variables.go 79.92% <0%> (-0.36%) ⬇️
util.go 92.68% <0%> (+2.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f677390...6c7d95e. Read the comment docs.

Comment thread initializer_test.go
@blakerouse
Copy link
Copy Markdown
Contributor Author

@urso I did some more work on the PR to allow nested structures that are not defined in the configuration to still have InitDefaults to be called.

I am still having issues with getting the primitive to work properly. The InitDefaults is called on myIntInitializer that is defined in the tests, but the value is never actually set on the structure it remains the default value.

I think the issue is in tryInitDefaults:

if t.Implements(iInitializer) {
	initializer = val.Interface().(Initializer)
} else if reflect.PtrTo(t).Implements(iInitializer) {
        // reaches this branch for the myIntInitializer
	val = pointerize(reflect.PtrTo(t), t, val)
	initializer = val.Interface().(Initializer)
}
if initializer != nil {
        // calls InitDefaults
	initializer.InitDefaults()

        // val is still reflect.Zero at this point
}

I think the issue is how the field is pointerized and the value is not actually set in the address location that is defined in the structure.

@blakerouse
Copy link
Copy Markdown
Contributor Author

@urso This is ready for another review.

Comment thread initializer.go
Comment thread reify.go
Comment thread reify.go Outdated
Comment thread types.go Outdated
Comment thread types.go Outdated
Comment thread unpack_test.go Outdated
@blakerouse
Copy link
Copy Markdown
Contributor Author

@urso I have removed the need for cfgEmpty and cfgSubFlex. The behavior for pointer types in structs has been implemented.

To completely remove the need for cfgEmpty I did have to add a short circuit to unpackWith where in the case the value is nil it just returns nil and no error.

Comment thread reify.go Outdated
@blakerouse blakerouse merged commit 4fd3937 into elastic:master Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initialize defaults in sub-structures

5 participants