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

proposal: net/url: implement TextMarshaler/TextUnmarshaler interfaces for url.URL #52638

Closed
YaroslavDev opened this issue May 1, 2022 · 10 comments

Comments

@YaroslavDev
Copy link

In order to be able to pass url.URL variable to flag.TextVar(&url, ...), url.URL type must implement encoding.TextMarshaler and encoding.TextUnmarshaler interfaces.
This approach was suggested here instead of making a dedicated flag.URLVar function in flag package.

@gopherbot gopherbot added this to the Proposal milestone May 1, 2022
@YaroslavDev YaroslavDev changed the title proposal: net/url: Implement TextMarshaler/TextUnmarshaler interfaces for url.URL proposal: net/url: implement TextMarshaler/TextUnmarshaler interfaces for url.URL May 1, 2022
@earthboundkid
Copy link
Contributor

I think this would be good, but is it a breaking change? Right now, the output of this code

	u, _ := url.Parse("http://example.com")
	b, _ := json.Marshal(u)
	fmt.Println(string(b))

is

{
 "ForceQuery": false,
 "Fragment": "",
 "Host": "example.com",
 "Opaque": "",
 "Path": "",
 "RawFragment": "",
 "RawPath": "",
 "RawQuery": "",
 "Scheme": "http",
 "User": null
}

I think it would be better if the output were "http://example.com" but maybe someone somewhere would miss the old output?

@YaroslavDev
Copy link
Author

@carlmjohnson nice catch!
I guess there are 2 options:

  1. Make a breaking change since url.URL typically is not used as JSON(maybe only for debugging/logging purposes)
  2. Implement explicitly json.Marshaler and json.Unmarshaler interfaces on url.URL as well to maintain backward compatibility

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 2, 2022
@YaroslavDev

This comment was marked as off-topic.

@seankhliao

This comment was marked as off-topic.

@earthboundkid

This comment was marked as off-topic.

@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 8, 2022

It seems like JSON decoding the { } form might still work since that's not a JSON string, so it won't try the TextUnmarshaler. So an old encoding might be readable by a new Go where URL implements these methods. The reverse would not be true.

Have we decided before whether this is OK? I seem to remember JSON encoding of IP addresses used to be base64 byte encodings, but I think we decided that was just so broken we could fix it.

Not changing the JSON seems like a big inconsistency too since I assume the goal here is to get better JSON support for URLs (in addition to flag support).

@seankhliao
Copy link
Member

It seems like JSON decoding the { } form might still work since that's not a JSON string, so it won't try the TextUnmarshaler.

encoding/json currently doesn't have that kind of fallback: https://go.dev/play/p/z8QXBU6Ov2z

json: cannot unmarshal object into Go value of type *main.New


Have we decided before whether this is OK?

#5086 for net.IP: done in go1.2
#10244 for net.HardwareAddr: declined for no standard format
#10275 for stdlib types in general: undecided
#12803 for net.IPNet: Not ok
#16039 for time.Duration: declined for no standard format
#24174 for time.Duration: folded into #10275
#25705 for url.URL: folded into #10275
#29678 for net.HardwareAddr: undecided
#40436 for net.IPMask: no comments

@rsc
Copy link
Contributor

rsc commented Jun 15, 2022

Thanks very much for that list, @seankhliao! It looks like we should merge this one into #10275 too, and then bring 10275 over to the non-Go2 proposal process.

@rsc rsc moved this from Active to Declined in Proposals (old) Jun 15, 2022
@rsc
Copy link
Contributor

rsc commented Jun 15, 2022

This proposal is a duplicate of a previously discussed proposal, as noted above,
and there is no significant new information to justify reopening the discussion.
The issue has therefore been declined as a duplicate.
— rsc for the proposal review group

@rsc rsc closed this as completed Jun 15, 2022
@golang golang locked and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants