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

Set mapping keys to "decoded" when custom unmarshaler is used #426

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mvo5
Copy link

@mvo5 mvo5 commented Sep 24, 2024

decode: set all keys under a mapping with custom marshaler
decoded

This is a naive fix for the issue of custom unmarshal and marking
keys as "decoded". It simply assumes that if there was a custom
unmarshal for a mapping type all keys got handlded by the custom
unmarshal code.

This might be too naive but it seems reasonable and it also seems
we would need a richer interface for a custom unmarshal that gives
access to MetaData if we want to be more fine grained.


decode: add (failing) test for undecoded fields in a struct

This commit adds a failing test for the scenario decribed in issue
#425

This commit adds a failing test for the scenario decribed in issue
BurntSushi#425
This is a naive fix for the issue of custom unmarshal and marking
keys as "decoded". It simply assumes that if there was a custom
unmarshal for a mapping type all keys got handlded by the custom
unmarshal code.

This might be too naive but it seems reasonable and it also seems
we would need a richer interface for a custom unmarshal that gives
access to `MetaData` if we want to be more fine grained.
@arp242
Copy link
Collaborator

arp242 commented Sep 25, 2024

This seems like a reasonable change.

Could you add a more extensive test though? At the very least something like:

a   = 1
arr = [2]

[tbl]
b = 3

inline = {c = 4}

Just so that keys in different locations are tested: array, table, inline table. Maybe a few more? I'm not sure if these are different code-paths off-hand, but doesn't hurt to test it.

This commit expands the test `TestDecodeCustomStructMarkedDecoded`
so that nested data is also tested (thanks to  Martin Tournoij).

This lead to an update of the code to also mark all nested keys as
decoded when a custom `UnmarshalTOML(data any)` interface is used.

It is the job of the implemenator of the UnmarshalTOML() to ensure
everything is decoded. An alternative would be to provide a new
`UnmarshalTOMLWithMetadata(data any, md *MetaData)` inteface that
would allow the implementaor of the custom unmarshaller to mark
fields as decoded. But it's unclear if that is needed because
a UnmarshalTOML() can already error if it "sees" unexpected or
missing data.
@mvo5
Copy link
Author

mvo5 commented Sep 26, 2024

Thanks a lot for your feedback! I updated the code and it now includes the more complex testcase that you suggested and that uncovered missing handling of nested which I added now too. I am happy to add more tests, but I need to think a bit what else to check for (I'm not super familiar with the toml details). Ideas welcome here of course :)

Please also let me know if I should squash or rebase my commits, I did them linearly mostly to make it easy to review but happy to do whatever is needed/preferred.

@mvo5 mvo5 changed the title [RFC] Set mapping keys to "decoded" when custom unmarshaler is used Set mapping keys to "decoded" when custom unmarshaler is used Sep 27, 2024
@mvo5
Copy link
Author

mvo5 commented Oct 8, 2024

Friendly ping, anything I can do or improve to help this along?

@arp242
Copy link
Collaborator

arp242 commented Oct 8, 2024

I'd like to carefully look at it and play around with it a bit. Not terrible time-consuming, but I don't have a lot of bandwidth for that right now. It's "on the list" and not forgotten, but it might be a bit before I have a chance to properly look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants