invidian commented 4 years ago

Currently, if non-empty struct is passed to DecodeBody function, decoding process will keep already initialized string fields values or overwrite them, if they are specified in HCL. This behaviour is useful, as it allows to have some default values for fields.

However, if the field is a block or multiple blocks (slice), then the entire block is overwritten, which erases the struct. Because of that, settings default values in nested structs is not possible.

Consider following HCL and struct example:

foo "foo" {
  nested {
    more = "foo"
  }
}

foo := &Foo{
  Plain: "foo",
  Nested: &Bar{
    Plain: "bar",
  },
}

The result will become:

foo := &Foo{
  Plain: "foo",
  Nested: &Bar{
    More: "foo",
  },
}

With this commit, decode functions will check will check if the value is nil and only then set them to empty struct, which allows for appending to existing structs.

In case of a slice, either new empty element will be added, or existing element will be used for setting new value.

Result after this patch:

foo := &Foo{
  Plain: "foo",
  Nested: &Bar{
    More: "foo",
    Plain: "bar,
  },
}

While it is not very useful for slices, since default values would have to be positional, it is useful for standalone blocks and it makes decoding process to behave more consistent.

Alternative to this patch would be always zeroing top-level struct when decode process starts, to maintain clear and consistent behaviour.

Signed-off-by: Mateusz Gozdek mateusz@kinvolk.io

invidian commented 4 years ago

Please let me know if any documentation changes are required for this PR.

Also, I wasn't sure where/how to add tests for that, so some help would be appreciated here, if tests for this are needed.

codecov-io commented 4 years ago

Codecov Report

Merging #120 into master will decrease coverage by <.01%. The diff coverage is 61.53%.

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
- Coverage   68.98%   68.98%   -0.01%     
==========================================
  Files          98       98              
  Lines       10324    10333       +9     
==========================================
+ Hits         7122     7128       +6     
- Misses       2873     2877       +4     
+ Partials      329      328       -1

Impacted Files	Coverage Δ
gohcl/decode.go	`82.53% <61.53%> (-0.8%)`	:arrow_down:

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 618463a...36b97e7. Read the comment docs.

hashibot commented 4 years ago

Hello! :robot:

Thank you for working on this change proposal.

Further development of HCL 2 will now be done in the main HCL repository. For reasons of compatibility with existing Go callers still using GOPATH mode, the master branch still tracks HCL 1 at this time, but we'll be continuing development of HCL 2 in a branch. For more information, please see the Version Selection wiki page.

Unfortunately, we can't robustly perform an automatic migration of pull requests into the new repository, and so my human friends have asked me to close out all of the existing PRs to reflect that sadly they cannot be merged in their current state. :confounded:

If you have the time and motivation, we'd love to continue discussion of this and the other existing proposed changes in the hashicorp/hcl repository in a new PR targeting the hcl2 branch. We're sorry that we weren't able to merge this prior to the transfer, and now cannot automatically migrate your PR to the new repository.

The hashicorp/hcl2 repository will soon be archived to reflect the fact that no new development is planned here. The repository will remain in place for the time being so that existing callers can continue to use it while they plan a migration to the new import paths and to a stable release of HCL 2.

hashicorp / hcl2

gohcl: retain blocks while decoding #120

Codecov Report