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

Fix UnmarshalTOML is not called when its underlying type is an array #870

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ImSingee
Copy link

This PR is for v1.

This resolves the issue where, if a type implements toml.Unmarshaler but it has an array as its underlying type, the UnmarshalTOML method won't be triggered.

@pelletier
Copy link
Owner

Hi! Thank you for the patch! However v1 is not supported anymore and will not receive any update. Consider using go-toml v2 instead.

@ImSingee
Copy link
Author

ImSingee commented May 19, 2023

@pelletier Thanks for yor reminder. You can close this PR at your opinion.

I’m not using v2 (in fact, I just downgraded to v1 today) because and only because of the lack of missing toml.Unmarshaler interface.

Though there’s TextUnmarshaler, but it can only handle string fields. However, I want to (and need to) do more for historical reasons. The case is just that we have a field that needs to accept both a string and a string array. But in v2, there’s no way to implement it.

Is it possible to re-add the toml.Unmarshaler interface? It will do increase flexibility. And if you accept the feature, I can submit a PR to implement that.

@pelletier
Copy link
Owner

Makes sense. I'd be happy to see a good patch to implement toml.Unmarshaler for v2! I think it would also help others (#857).

@ImSingee
Copy link
Author

@pelletier 👀 Okay, I will try to implement it!

@pelletier
Copy link
Owner

@ImSingee created #873 with some thoughts on the topic, hopefully it's helpful!

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