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

net/url: URL should implement UnmarshalText, not just UnmarshalBinary #25705

Closed
aviau opened this issue Jun 2, 2018 · 4 comments
Closed

net/url: URL should implement UnmarshalText, not just UnmarshalBinary #25705

aviau opened this issue Jun 2, 2018 · 4 comments

Comments

@aviau
Copy link

aviau commented Jun 2, 2018

The URL struct of net/url only implements UnmarshalBinary.

I think that it should also implement UnmarshalText (it would be the same code)

There are parsers that depend on this.

net/url UnmarshalBinary's signature even names the []byte argument "text":

func (u *URL) UnmarshalBinary(text []byte) error
@aviau aviau changed the title net/url: implement UnmarshalText net/url: URL should implement UnmarshalText, not just UnmarshalBinary Jun 2, 2018
@mvdan
Copy link
Member

mvdan commented Jun 2, 2018

Have you had a look at the source code?

// Marshaling interface implementations.
// Would like to implement MarshalText/UnmarshalText but that will change the JSON representation of URLs.

func (u *URL) MarshalBinary() (text []byte, err error) {
        return []byte(u.String()), nil
}

func (u *URL) UnmarshalBinary(text []byte) error {
        u1, err := Parse(string(text))
        if err != nil {
                return err
        }
        *u = *u1
        return nil
}

In particular, read the commit that added this code - 6595709. It seems like adding what you suggest would break existing code. Perhaps this can be left for Go2, or for any other opportunity where backwards compatibility can be broken in this package.

@ghost
Copy link

ghost commented Jun 2, 2018

Duplicate #10275

@mvdan
Copy link
Member

mvdan commented Jun 2, 2018

Thanks, @bontibon. It indeed makes sense to group all of these under one umbrella issue for Go2.

@mvdan mvdan closed this as completed Jun 2, 2018
@aviau
Copy link
Author

aviau commented Jun 4, 2018

It seems like adding what you suggest would break existing code.

Why? I don't understand.

Would like to implement MarshalText/UnmarshalText but that will change the JSON representation of URLs.

Only implementing UnmarshalText wouldn't change the JSON representation of URLs.

What is wrong with only implementing the parsing portion?

@golang golang locked and limited conversation to collaborators Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants