Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

marshal: do not encode embedded structs as sub-table #368

Merged
merged 3 commits into from
Apr 25, 2020

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Apr 24, 2020

Currently, the marshalling code encodes the embedded structs as sub-tables.
This is a bit unexpected, as it differs from what encoding/json does in
that case: https://play.golang.org/p/KDPaGtrijV1

This PR adapts the encoder to behave like encoding/json.
Fields in an embedded struct are promoted to the top level table.
In case the embedded struct is named in the tag, it will still
encode as a sub-table.

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #368 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   95.08%   95.12%   +0.03%     
==========================================
  Files          10       10              
  Lines        2118     2134      +16     
==========================================
+ Hits         2014     2030      +16     
  Misses         64       64              
  Partials       40       40              
Impacted Files Coverage Δ
marshal.go 94.47% <100.00%> (+0.16%) ⬆️

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 947ab3f...cc3ccf2. Read the comment docs.

@oncilla
Copy link
Contributor Author

oncilla commented Apr 24, 2020

Hi, please let me know if you agree with this approach.
If so, I will be happy to update the docs accordingly.

In case we cannot do this, because it is a breaking change, we could also add another option to the tag. Something like toml:",embed".

@pelletier
Copy link
Owner

Great catch! It is a breaking change, but I see it like a bugfix (it was not supposed to do this in the first place).

If you wouldn't mind, I'd love to this is as the default behavior, but maybe with an option on Encoder to be able to revert to he previous behavior in case people depend on it (I think a per-field flag is overkill).

@pelletier pelletier added the bug Issues describing a bug in go-toml. label Apr 25, 2020
@oncilla
Copy link
Contributor Author

oncilla commented Apr 25, 2020

Alright. I updated the code with a PromoteAnonymous method on the encoder.
I struggled a bit with naming it, if you have a better suggestion, let me know.

I also changed the behavior on handling duplicate keys. It is now in line with encoding/json.
I.e., for duplicate keys, the regular field shadows the field from the anonymous struct.

This should now be ready to review.

oncilla added 3 commits April 25, 2020 11:02
Currently, the marshalling code encodes the embedded structs as sub-tables.
This is a bit unexpected, as it differs from what encoding/json does in
that case: https://play.golang.org/p/KDPaGtrijV1

Unmarshalling code handles this scenario gracefully.

This PR adapts the encoder to behave like encoding/json.
Fields in an embedded struct are promoted to the top level table.
In case the embedded struct is named in the tag, it will still
encode as a sub-table.
The added PromoteAnonymous option on the Encoder allows configuring
the old behavior, where anonymous structs are encoded as sub-tables.

On duplicate keys, the behavior of encoding/json is mimicked:
Fields from anonymous structs are shadowed by regular fields.

An example is added to show the affects of setting PromoteAnonymous.
@pelletier
Copy link
Owner

Name looks good, patch is great! Thanks a lot for taking care of this.

@pelletier pelletier merged commit d1e0fc3 into pelletier:master Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues describing a bug in go-toml.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants