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

Implement generic text encoding interfaces #163

Closed

Conversation

chrisnovakovic
Copy link

@chrisnovakovic chrisnovakovic commented Apr 21, 2021

Implement the yaml.Marshaler and yaml.Unmarshaler interfaces from gopkg.in/yaml.v2 in Version.

(I don't know whether there's any appetite for merging marshalers for data serialisation formats that aren't in the standard library, but a YAML one is something I have a need for and I figured it'd be worth opening a PR for it just in case. This only introduces a dependency on gopkg.in/yaml.v2 in the tests.)

Rather than implementing encoding/json's Marshaler and Unmarshaler interfaces in Version, implement encoding's TextMarshaler and TextUnmarshaler interfaces. These interfaces are used by encoding/json as well as multiple third-party encoding/decoding packages.

Implement the `yaml.Marshaler` and `yaml.Unmarshaler` interfaces from
`gopkg.in/yaml.v2` in `Version`.
@pamburus
Copy link

It would be better to implement encoding.TextMarshaler and encoding.TextUnmarshaler interfaces.
They are in standard library and they will be used in any text marshaling, including YAML, JSON, and a lot of others.

@chrisnovakovic
Copy link
Author

It would be better to implement encoding.TextMarshaler and encoding.TextUnmarshaler interfaces. They are in standard library and they will be used in any text marshaling, including YAML, JSON, and a lot of others.

Quite right, thanks! I'll push a new commit in a second...

@chrisnovakovic chrisnovakovic changed the title Implement go-yaml's Marshaler and Unmarshaler interfaces Implement generic text encoding interfaces Apr 16, 2022
Copy link
Member

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

Unfortunately, this is a breaking API change. Removing functions will do that. The Text ones would need to be additive.

@MarkRosemaker
Copy link
Contributor

I've added them instead of replacing them in this PR and also added marshalling and unmarshalling tests for three yaml libraries:
#173

@mattfarina
Copy link
Member

I'm closing this one in light of #173 since removing functions would change the public API which we aren't doing in a minor improvement.

@mattfarina mattfarina closed this Aug 9, 2022
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

4 participants