code review 5672072: net/url: API

net/url: API

Convert cryptotype to general go1rename fix.
Add os.Exec -> syscall.Exec fix along with new
URL fixes.

Fixes issue 2946.

[rsc, 2012-02-16 18:22:38]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2012-02-16 19:52:42]

LGTM
works for me, but dsymonds didn't like ParseFragment. i'll let you two fight
that out.

[rsc, 2012-02-16 20:00:24]

That's fine.  I dropped with With because there is nothing for it to modify.

[dsymonds, 2012-02-16 21:25:28]

On Fri, Feb 17, 2012 at 7:00 AM, Russ Cox <rsc@golang.org> wrote:

> That's fine.  I dropped with With because there is nothing for it to modify.

I explained why I included the With in the previous CL.
"ParseFragment" is incorrect; it says that it parses a fragment, which
it does not. Also, in the caller's context, it will be
url.ParseWithFragment, so the "url" gives a context for the With. I
could make it ParseURLWithFragment if you're really concerned about
the With being ambiguous, but that seems unnecessary.

Fragments with structure are getting more and more common these days.
It's reasonable to parse one. So saying "ParseFragment" is, I believe,
misleading. I don't see the With being an unbearably imposition to
avoid the confusion.


Dave.

[rsc, 2012-02-16 21:26:59]

On Thu, Feb 16, 2012 at 16:25, David Symonds <dsymonds@golang.org> wrote:
> I explained why I included the With in the previous CL.
> "ParseFragment" is incorrect; it says that it parses a fragment, which
> it does not. Also, in the caller's context, it will be
> url.ParseWithFragment, so the "url" gives a context for the With. I
> could make it ParseURLWithFragment if you're really concerned about
> the With being ambiguous, but that seems unnecessary.
>
> Fragments with structure are getting more and more common these days.
> It's reasonable to parse one. So saying "ParseFragment" is, I believe,
> misleading. I don't see the With being an unbearably imposition to
> avoid the confusion.

You don't parse with a fragment.  You parse with a parser.
Neither name is complete; names never are.
The extra word is not adding any value here.

Russ

[dsymonds, 2012-02-16 21:30:40]

On Fri, Feb 17, 2012 at 8:26 AM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Feb 16, 2012 at 16:25, David Symonds <dsymonds@golang.org> wrote:
>> I explained why I included the With in the previous CL.
>> "ParseFragment" is incorrect; it says that it parses a fragment, which
>> it does not. Also, in the caller's context, it will be
>> url.ParseWithFragment, so the "url" gives a context for the With. I
>> could make it ParseURLWithFragment if you're really concerned about
>> the With being ambiguous, but that seems unnecessary.
>>
>> Fragments with structure are getting more and more common these days.
>> It's reasonable to parse one. So saying "ParseFragment" is, I believe,
>> misleading. I don't see the With being an unbearably imposition to
>> avoid the confusion.
>
> You don't parse with a fragment.  You parse with a parser.
> Neither name is complete; names never are.
> The extra word is not adding any value here.

I just explained the extra value.

I'd take ParseURLWithFragment over ParseFragment any day. The latter is wrong.


Dave.

[rsc, 2012-02-16 21:31:20]

On Thu, Feb 16, 2012 at 16:30, David Symonds <dsymonds@golang.org> wrote:
> I just explained the extra value.
>
> I'd take ParseURLWithFragment over ParseFragment any day. The latter is wrong.

I think ParseWithFragment and ParseFragment are equally
incomplete.  Given that, the shorter one wins.

url.ParseURLWithFragment is a Java function.

[rsc, 2012-02-16 21:31:38]

On Thu, Feb 16, 2012 at 16:31, Russ Cox <rsc@golang.org> wrote:
> url.ParseURLWithFragment is a Java function.

and it still sounds like it is using a fragment to do the parsing.

[dsymonds, 2012-02-16 21:40:30]

Different thought: let's drop this function altogether. Parse can
easily do both jobs. It's not like we have a ParsePort, or
ParseUserPassword, or ParseURLWithQuery or something.


Dave.

[rsc, 2012-02-16 21:49:27]

On Thu, Feb 16, 2012 at 16:40, David Symonds <dsymonds@golang.org> wrote:
> Different thought: let's drop this function altogether. Parse can
> easily do both jobs. It's not like we have a ParsePort, or
> ParseUserPassword, or ParseURLWithQuery or something.

I am not sure that Parse should do both jobs.
Most URLs passed around on the network
should not have #fragment, so it is important
to return an error if they do.  For example,
it is important not to send the #fragment in
a GET request.

ParseFragment was supposed to be something
that you'd use if you were inside a web browser,
and Parse if you were getting URLs from the
network.  However, we had to add ParseRequestURI
for reasons I have forgotten.  Maybe the people
who don't want to allow #fragment are already calling
ParseRequestURI, so that we can make Parse be
the old ParseReference?

Russ

[dsymonds, 2012-02-16 22:02:25]

On Fri, Feb 17, 2012 at 8:49 AM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Feb 16, 2012 at 16:40, David Symonds <dsymonds@golang.org> wrote:
>> Different thought: let's drop this function altogether. Parse can
>> easily do both jobs. It's not like we have a ParsePort, or
>> ParseUserPassword, or ParseURLWithQuery or something.
>
> I am not sure that Parse should do both jobs.
> Most URLs passed around on the network
> should not have #fragment, so it is important
> to return an error if they do.  For example,
> it is important not to send the #fragment in
> a GET request.

A URL with a fragment is still a URL. The fact that in certain
contexts its presence is incorrect implies that checking for it should
probably be done in those places, not in a general URL parser.


> ParseFragment was supposed to be something
> that you'd use if you were inside a web browser,
> and Parse if you were getting URLs from the
> network.  However, we had to add ParseRequestURI
> for reasons I have forgotten.  Maybe the people
> who don't want to allow #fragment are already calling
> ParseRequestURI, so that we can make Parse be
> the old ParseReference?

Yeah, I can't understand why ParseRequestURI is there either.


Dave.

[rsc, 2012-02-16 22:27:22]

On Thu, Feb 16, 2012 at 17:02, David Symonds <dsymonds@golang.org> wrote:
> Yeah, I can't understand why ParseRequestURI is there either.

At least that one I can explain.
Parse and ParseRequestURI can return different results
on the same input.  One expects a URL like would be
in an HTML page (without #fragment) and the other
expects what you'd get in an HTTP request.
The grammars are different and neither is a subset
of the other:

$ cat x.go
package main

import (
	"fmt"
	"net/url"
)

func main() {
	const x = "//not.a.user@not.a.host/just/a/path"
	
	u, _ := url.Parse(x)
	fmt.Printf("Parse: %+v\n", *u)

	u, _ = url.ParseRequestURI(x)
	fmt.Printf("ParseRequestURI: %+v\n", *u)
}

$ go run x.go
Parse: {Scheme: Opaque: User:not.a.user Host:not.a.host
Path:/just/a/path RawQuery: Fragment:}
ParseRequestURI: {Scheme: Opaque: User:<nil> Host:
Path://not.a.user@not.a.host/just/a/path RawQuery: Fragment:}
$

ParseFragment and Parse similarly accept the same inputs but return
different results.

$ cat x.go
package main

import (
	"fmt"
	"net/url"
)

func main() {
	const x = "http://www.google.com/#foo"
	
	u, _ := url.Parse(x)
	fmt.Printf("Parse: %+v\n", *u)

	u, _ = url.ParseFragment(x)
	fmt.Printf("ParseFragment: %+v\n", *u)

	u, _ = url.ParseRequestURI(x)
	fmt.Printf("ParseRequestURI: %+v\n", *u)
}

$ go run x.go
Parse: {Scheme:http Opaque: User:<nil> Host:www.google.com Path:/#foo
RawQuery: Fragment:}
ParseFragment: {Scheme:http Opaque: User:<nil> Host:www.google.com
Path:/ RawQuery: Fragment:foo}
ParseRequestURI: {Scheme:http Opaque: User:<nil> Host:www.google.com
Path:/#foo RawQuery: Fragment:}
$

What I don't know is whether all the cases where you want the
current fragment-free Parse should be using ParseRequestURI
instead.  If so, we can delete the current Parse and rename
ParseFragment to Parse.  But that's a big if.

Russ

[dsymonds, 2012-02-16 23:06:05]

On Fri, Feb 17, 2012 at 9:27 AM, Russ Cox <rsc@golang.org> wrote:

> What I don't know is whether all the cases where you want the
> current fragment-free Parse should be using ParseRequestURI
> instead.  If so, we can delete the current Parse and rename
> ParseFragment to Parse.  But that's a big if.

I've convinced myself that we can. URLs without fragments currently
behave the same in both paths, so we only have to consider URLs with
fragments. Doing this reshuffle would change code that currently
expects to get an error for a URL-with-fragment to no longer get an
error, but still get a valid URL. Sensible code would not be looking
in the Fragment field after that, so the rest of their code should
still be valid. As you said, they typically aren't passed around a
network, and it's relatively rare to be dealing with them, so this is
a rare case that now causes code to work instead of erroring out. That
seems fine to me (though worth noting in the release notes), and
definitely worth the API simplification.


Dave.

[rsc, 2012-02-16 23:38:37]

Okay.  I'll revise this CL.

[rsc, 2012-02-17 02:52:13]

Hello golang-dev@googlegroups.com, r@golang.org, dsymonds@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2012-02-17 03:37:45]

LGTM

I am happy, thanks.

[rsc, 2012-02-17 04:46:31]

*** Submitted as 789b1662e5ce ***

net/url: API

Convert cryptotype to general go1rename fix.
Add os.Exec -> syscall.Exec fix along with new
URL fixes.

Fixes issue 2946.

R=golang-dev, r, dsymonds
CC=golang-dev
http://codereview.appspot.com/5672072