Closed GregoryAlbouy closed 1 year ago
Merging #61 (861609c) into main (a729645) will increase coverage by
0.47%
. The diff coverage is83.33%
.:exclamation: Current head 861609c differs from pull request most recent head 39fb4a6. Consider uploading reports for the commit 39fb4a6 to get more accurate results
:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 81.16% 81.64% +0.47%
==========================================
Files 28 29 +1
Lines 945 1024 +79
==========================================
+ Hits 767 836 +69
- Misses 147 150 +3
- Partials 31 38 +7
Flag | Coverage Δ | |
---|---|---|
unittests | 81.64% <83.33%> (+0.47%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
configio/decoder.go | 63.63% <50.00%> (-14.15%) |
:arrow_down: |
configio/yaml.go | 91.48% <75.00%> (+5.48%) |
:arrow_up: |
configio/builder.go | 82.71% <82.71%> (ø) |
|
configio/representation.go | 68.54% <85.71%> (+0.51%) |
:arrow_up: |
configio/file.go | 92.59% <100.00%> (ø) |
|
configio/json.go | 94.44% <100.00%> (+7.26%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Description
Meant to replace configio.Representation for external use. configio.Representation is hardly usable because of nested structs and pointers: it should remain an internal unmarshaling/marshaling structure only.
Problem statement
A common use-case we have is the need to combine benchttp configuration, by overriding successively different sources (e.g. in benchttp/cli: DefaultRunner <- configuration file <- cli flags). To do this, we need to know which fields are set and which are not for each override. However, due to Go initializing unset fields with zero values, it's impossible to determine whether a zero-value field was actually set or defaulted by Go.
Consecutive solutions
➡️ Solution 1:
runner.Config.Override
To overcome this we first added memorizing capabilities to
runner.Config
, which used with its methodOverride
allowed to perform this kind of selective override. This solution was far from ideal: it was cumbersome to use for the consumer (having to keep track of set fields), it polluted packagerunner
with manyConfigFieldXxx
declarations and added undesirable complexity to the package.➡️ Solution 2:
configio.Representation
(previously
configparse.Representation
)In https://github.com/benchttp/engine/pull/58, we nuke the
runner.Config
structure and its overriding logics. Instead we take advantage ofconfigio.Representation
: previously used as a decoding structure only, it uses pointer fields to distinguish zero values from undefined values. This combined with the adoptions ofMarshal
-like signature provides a functional overriding logic:Overriding a
```go var jsonConfig = []byte(`"runner":{"requests": 42}`) func main() { // start with default runner as base runner := benchttp.DefaultRunner() // parse json config as configio.Representation repr := configio.Representation{} _ = configio.Unmarshal(jsonConfig, &repr) // override default with json config _ = repr.Into(&runner) } ```benchttp.Runner
with json config usingconfigio.Representation
This works well for what it was initially intended for: decoding bytes. Other than that it turns out to be extremely painful to manipulate, due to nested structs and pointer fields. As such, it ties overriding logics to the action of decoding. Let's try to assign a value to
configio.Representation.Body
(e.g. in a testing context or from anybenchttp
consumer point of view):Cumbersome manipulation of
```go package main import "github.com/benchttp/sdk/configio" func main() { repr := configio.Representation{} // Nested structs must be redefined repr.Request.Body = &struct { // Yes, including struct tags Type string `yaml:"type" json:"type"` Content string `yaml:"content" json:"content"` }{ Type: "raw", Content: "hello", } // Pointer values must be stored in a variable first // repr.Request.Method = "GET" // ^^^^^ // cannot use "GET" (untyped string constant) as *string value requestMethod := "GET" repr.Request.Method = &requestMethod runnerConcurrency := 10 repr.Runner.Concurrency = &runnerConcurrency } ```configio.Representation
This shows how unadapted
configio.Representation
is for the general purpose of building a config. It is tied to the purpose of decoding a JSON/YAML config and as such it must remain an internal decoding structure.➡️ Solution 3:
configio.Builder
The purpose of
configio.Builder
is to provide a building API forbenchttp.Runner
.strings.Builders
) using write and setter methods (b.WriteJSON
,b.SetConcurrency
)configio.Representation
for that matterThis makes
configio.Representation
obsolete as an exported value, but still needed as an internal decoding structure.That's why we also unexport
configio.Representation
and update all related public function to work withconfigio.Builder
instead.Changes
configio.Builder
configio.Builder
instead ofconfigio.Representation
configio.Representation
Notes