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

Ensure that YAML marshalling matches the JSON marshalling #943

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

Conversation

damien-talos
Copy link

I noticed that some types used MarshalYAML as the source of truth for MarshalJSON, but others didn't.
Some of those were out of sync, especially with regards to the serialization of extensions.

This PR updates all the MarshalJSON functions to call the MarshalYAML one, with the result that both JSON and YAML marshalling should result in the same value being written.

@fenollp
Copy link
Collaborator

fenollp commented Apr 28, 2024

Hi! Thanks for submitting this. Should we be thinking of a regression test?
Please rebase your PR, otherwise LGTM :)

@damien-talos
Copy link
Author

@fenollp Certainly, I'll rebase my PR.
I'm happy to add tests, though I'm not sure what that would look like?
Perhaps somehow generating tests that serialize various objects to both JSON and YAML, than de-serialize them, and compare them to ensure they are equivalent? Is there existing code to generate tests like that, or would they have to be hand-coded?

@fenollp
Copy link
Collaborator

fenollp commented Apr 29, 2024

Such tests already exist, I was just wondering if you'd thought of something new. No worries, let's just rebase :)

@damien-talos
Copy link
Author

I did a rebase here, and fixed a few issues, but I've now got tests that are failing because the properties are serialized in a different order.
How important is the order of the properties in the serialized output? I could update the snapshots to reflect the new order (which seems to be alphabetical), but I'm not sure if that's desired or not?

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.

None yet

2 participants