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

all: decide what can implement TextMarshaler/TextUnmarshaler #10275

Closed
lavalamp opened this issue Mar 27, 2015 · 20 comments
Closed

all: decide what can implement TextMarshaler/TextUnmarshaler #10275

lavalamp opened this issue Mar 27, 2015 · 20 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Milestone

Comments

@lavalamp
Copy link

For example, the standard library has a decent encoding/json package, and a great time.Duration object. But these objects don't play well together; if you want to encode a Duration in a json blob, you'll have to make a custom type that calls .String()/ParseDuration() when Get/SetJSON are called. It would be nice if they worked together by default.

...Unfortunately, the cross product of all the useful encoding packages with all the useful types in the standard library is probably quite large, so I'll understand if you say "no" to that. Perhaps at least time.Time and time.Duration could be supported?

@mikioh mikioh changed the title Feature request: std lib: common types to support common encodings all: common types to support common encodings Mar 27, 2015
@rsc
Copy link
Contributor

rsc commented Apr 10, 2015

This isn't possible to change now, as it would change the encodings produced by programs that exist today.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc added the v2 A language change or incompatible library change label Apr 10, 2015
@lavalamp
Copy link
Author

@rsc Let me push back just a little on that, I don't see why it's necessarily true.

For example, again taking encoding/json with time.Time & Duration: I don't see a reason why the time.SetJSON method couldn't accept as input the string that's currently emitted.

I can see why you'd want to change the output of time.Duration, but I'm not sure why it'd be a problem to make the SetJSON method accept both the new output format (e.g., "5s") and the old (which is nanoseconds, without quotes). Those cases are super easy to distinguish.

...I agree the generalized cross product of types w/ encodings is probably going to have many much more difficult cases.

@rsc
Copy link
Contributor

rsc commented Jun 16, 2017

Replying to a comment from 2015, but we can't change the encoding outputs now because then if a new version of Go is sending data to an older version, the older version will not understand it.

@rsc rsc changed the title all: common types to support common encodings encoding: make stdlib types implement TextMarshaler/BinaryMarshaler consistently Jun 16, 2017
@lavalamp
Copy link
Author

That means it'd have to be opt in, e.g. with paired type TextMarshaller2 interface { ... V2() } and json.NewEncoder().UseV2().Encode(). Obviously that's hideous and I don't think it should be done, but it is possible.

@rsc
Copy link
Contributor

rsc commented Jun 17, 2017

It means it would have to wait for Go 2.

@rsc rsc changed the title encoding: make stdlib types implement TextMarshaler/BinaryMarshaler consistently proposal: encoding: make stdlib types implement TextMarshaler/BinaryMarshaler consistently Jun 17, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Jan 9, 2018
@ianlancetaylor
Copy link
Contributor

If we change this in Go 2--if we add marshaling methods for standard library types--we will have to provide some mechanism for Go 2 programs to read Go 1 data. One approach would be to only add marshaling methods in new versions of the packages, and to permit the old and new versions of the package to both be imported in the same program.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 9, 2018
@aviau
Copy link

aviau commented Jun 7, 2018

we can't change the encoding outputs now

@rsc Would you consider implementing Unmarshal interfaces?

We can do this right now, can't we?

#25705 Was closed because apparently implementing UnmarshalText in net/url would break existing code. Maybe I am missing it, so please correct me if I am wrong, but I don't see how that is true.

There are plenty of use cases for code that parses URLs and this seems like an easy fix.

@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 16, 2019
@gopherbot gopherbot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 3, 2019
@andig
Copy link
Contributor

andig commented Feb 28, 2021

@rsc Would you consider implementing Unmarshal interfaces?

Ping @rsc would that be possible without breaking compatibility?

iwilltry42 added a commit to k3d-io/k3d that referenced this issue Oct 13, 2021
- add validation as extra step after migration
- add missing json struct tags to new v1alpha3 config fields
- add unit tests
- add missing `omitempty` tags to configs to avoid (un-)marshalling issues
- drop `string` type for `time.Duration` type field in JSON Schema (doesn't work properly sometimes due to broken handling of time.Duration as per golang/go#10275)
@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

Moving to 'likely accept'. The implementation can be adding a comment to TextMarshaler/TextUnmarshaler, which I can send.

@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Aug 3, 2022
@rsc rsc changed the title proposal: all: decide what can implement TextMarshaler/TextUnmarshaler all: decide what can implement TextMarshaler/TextUnmarshaler Aug 12, 2022
@rsc rsc modified the milestones: Proposal, Backlog Aug 12, 2022
@rsc
Copy link
Contributor

rsc commented Aug 12, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@gopherbot
Copy link

Change https://go.dev/cl/496537 mentions this issue: encoding: document when marshaling methods can be added

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 30, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 May 30, 2023
jacobbednarz added a commit to jacobbednarz/cloudflare-go that referenced this issue Jun 8, 2023
All Tunnel timeout values are based on seconds however, there isn't a great way
to do this in a flexible way with Go due to `time.Duration` not having
marshal/unmarshal support in Go 1[1]. Instead, we introduce a custom
`TunnelDuration` value here and ensure it always converts durations into
seconds without impacting other users of `time.Duration`.

[1]: golang/go#10275
jacobbednarz added a commit to jacobbednarz/cloudflare-go that referenced this issue Jun 8, 2023
All Tunnel timeout values are based on seconds however, there isn't a great way
to do this in a flexible way with Go due to `time.Duration` not having
marshal/unmarshal support in Go 1[1]. Instead, we introduce a custom
`TunnelDuration` value here and ensure it always converts durations into
seconds without impacting other users of `time.Duration`.

[1]: golang/go#10275
jacobbednarz added a commit to jacobbednarz/cloudflare-go that referenced this issue Jun 8, 2023
All Tunnel timeout values are based on seconds however, there isn't a great way
to do this in a flexible way with Go due to `time.Duration` not having
marshal/unmarshal support in Go 1[1]. Instead, we introduce a custom
`TunnelDuration` value here and ensure it always converts durations into
seconds without impacting other users of `time.Duration`.

[1]: golang/go#10275
jacobbednarz added a commit to cloudflare/terraform-provider-cloudflare that referenced this issue Jun 8, 2023
Due to marshal/unmarshal support lacking in Go 1[1], we were previously
sending nanosecond values to the API instead of the required seconds.
This would have resulted in higher than usual timeout durations (10
seconds would have been sent as 1000000000 seconds since it duration
unmarshals to nanoseconds).

Updates all the instances of time duration handling in
`cloudflare_tunnel_config` resource to use the newly introduced
`TunnelDuration` to ensure we correctly setting the units.

[1]: golang/go#10275
jacobbednarz added a commit to cloudflare/terraform-provider-cloudflare that referenced this issue Jun 8, 2023
Due to marshal/unmarshal support lacking in Go 1[1], we were previously
sending nanosecond values to the API instead of the required seconds.
This would have resulted in higher than usual timeout durations (10
seconds would have been sent as 1000000000 seconds since it duration
unmarshals to nanoseconds).

Updates all the instances of time duration handling in
`cloudflare_tunnel_config` resource to use the newly introduced
`TunnelDuration` to ensure we correctly setting the units.

[1]: golang/go#10275
jacobbednarz added a commit to jacobbednarz/cloudflare-go that referenced this issue Jun 8, 2023
All Tunnel timeout values are based on seconds however, there isn't a great way
to do this in a flexible way with Go due to `time.Duration` not having
marshal/unmarshal support in Go 1[1]. Instead, we introduce a custom
`TunnelDuration` value here and ensure it always converts durations into
seconds without impacting other users of `time.Duration`.

[1]: golang/go#10275
jacobbednarz added a commit to cloudflare/terraform-provider-cloudflare that referenced this issue Jun 22, 2023
Due to marshal/unmarshal support lacking in Go 1[1], we were previously
sending nanosecond values to the API instead of the required seconds.
This would have resulted in higher than usual timeout durations (10
seconds would have been sent as 1000000000 seconds since it duration
unmarshals to nanoseconds).

Updates all the instances of time duration handling in
`cloudflare_tunnel_config` resource to use the newly introduced
`TunnelDuration` to ensure we correctly setting the units.

[1]: golang/go#10275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Status: Accepted
Proposals (old)
Likely Accept
Development

No branches or pull requests

10 participants