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

feat: Switch back to native yaml decoding #257

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

nieomylnieja
Copy link
Collaborator

@nieomylnieja nieomylnieja commented Feb 3, 2024

Motivation

When recently working on Data Export, we've encountered a problem using json.RawMessage with go-yaml decoding.
The raw message was used to postpone decoding of Data Export spec until we could decide what type should it be decoded to.
Due to time limitations back then we went with the easier solution and decided to use yaml.YAMLToJSON, which converts YAML to JSON and then uses encoding/json to decode the spec.

This is not ideal.
Doing it this way we're adding a decoding overhead, that's one thing.
Another, more serious issue is that we're stripping ourselves from receiving detailed syntax errors for YAML.
Instead, we're always getting JSON errors.

By adding serdeutil package and RawMessage which implements both JSON and YAML (un)marshalers we're bringing the aforementioned capabilities back home.

Release Notes

Syntax errors when parsing YAML now return more specific details.

@nieomylnieja
Copy link
Collaborator Author

As a side note, I can't really recall why this was a problem in first place:

		// Workaround for https://github.com/goccy/go-yaml/issues/313.
		// If the library changes its interpretation of empty pointer fields,
		// we should switch to native yaml.Unmarshal instead.

All the unit tests are passing, and revisiting the mentioned issue actually makes me wonder If it is in fact an issue 🤷

Copy link
Contributor

@daniel-zelazny daniel-zelazny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever solution 🚀

internal/serdeutil/raw_message.go Show resolved Hide resolved
internal/serdeutil/raw_message.go Show resolved Hide resolved
@nieomylnieja nieomylnieja merged commit 3c032db into main Feb 27, 2024
7 checks passed
@nieomylnieja nieomylnieja deleted the switch-back-to-native-yaml-decoding branch February 27, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants