code review 4893043: url: new package

url: new package
This is just moving the URL code from package http into its own package,
which has been planned for a while.
Besides clarity, this also breaks a nascent dependency cycle the new template
package was about to introduce.

Add a gofix module, url, and use it to generate changes outside http and url.

Sadness about the churn, gladness about some of the naming improvements.

[r, 2011-08-15 08:12:40]

Hello golang-dev@googlegroups.com,

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

[dsymonds, 2011-08-15 09:20:23]

LGTM

It's good to see this finally done.

Let's not get hung up on URI vs. URL, but I'd like s/uri/urlStr/ even for just
that the "i" in "uri" looks very similar to the "l" in "url" in many fonts.

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/client_test.go
File src/pkg/http/client_test.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/client_test.go#newc...
src/pkg/http/client_test.go:113: uri := "http://dummy.faketld/"
Please let's go with urlStr instead of uri.

To the nitpickers who care (such as me), URL and URI are quite different (URL is
a specific type of URI).

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/request.go#newcode650
src/pkg/http/request.go:650: for k, v := range newValues {
code elsewhere uses "vs" for a plurality of "v", so change v -> vs, and u -> v.

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/server.go#newcode720
src/pkg/http/server.go:720: func Redirect(w ResponseWriter, r *Request, uri
string, code int) {
uri -> urlStr

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/transport.go
File src/pkg/http/transport.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/transport.go#newcode93
src/pkg/http/transport.go:93: func ProxyURL(uri *url.URL) func(*Request)
(*url.URL, os.Error) {
this should probably just be s/uri/fixedURL/

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/transport.go#newcod...
src/pkg/http/transport.go:417: proxyURL     *url.URL   // "" for no proxy, else
full proxy URL
it was wrong before, but s/""/nil/ while you're here.

http://codereview.appspot.com/4893043/diff/1/src/pkg/url/url.go
File src/pkg/url/url.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/url/url.go#newcode5
src/pkg/url/url.go:5: // Package URL parses and escapes URLs (actually URIs).
to be more pedantic, although the objects that this package manipulates are
URIs, there are URIs that this package doesn't handle. It only handles the
subset of URIs called URLs.

http://codereview.appspot.com/4893043/diff/1/src/pkg/url/url.go#newcode187
src/pkg/url/url.go:187: // EncodedPath returns the URL's path in "URL path
encoded" form.
this method seems odd all the way up here. move it down below with the other
methods on *URL.

http://codereview.appspot.com/4893043/diff/1/src/pkg/url/url.go#newcode331
src/pkg/url/url.go:331: // ParseURL parses rawurl into a URL structure.
update name

http://codereview.appspot.com/4893043/diff/1/src/pkg/url/url.go#newcode496
src/pkg/url/url.go:496: // Unlike in the Header map, the keys in a Values map
s/Header/http.Header/

http://codereview.appspot.com/4893043/diff/1/src/pkg/url/url.go#newcode616
src/pkg/url/url.go:616: // ParseURL parses a URL in the context of a base URL. 
The URL in ref
update name

[bradfitz, 2011-08-15 15:02:18]

Nice.

This seems gofix-able?

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/http/request.go#newcode650
src/pkg/http/request.go:650: for k, v := range newValues {
On 2011/08/15 09:20:23, dsymonds wrote:
> code elsewhere uses "vs" for a plurality of "v", so change v -> vs, and u ->
v.

Yeah, either "vs" or "vv" (mnemonic: value vector) is used in this package a few
places.

"u" is just a weird name.

[rsc, 2011-08-15 16:25:51]

It's unfortunate that naming the package url means
you can't have a parameter or variable named url.
It will make gofix difficult too.

I am tempted to suggest naming the package uri
but that's probably more ugly.

[bradfitz, 2011-08-15 16:36:12]

On Mon, Aug 15, 2011 at 9:25 AM, Russ Cox <rsc@golang.org> wrote:

> It's unfortunate that naming the package url means
> you can't have a parameter or variable named url.
> It will make gofix difficult too.
>
> I am tempted to suggest naming the package uri
> but that's probably more ugly.
>

I wouldn't mind naming the package uri.

It's technically more correct and allows the use of url as a variable name.
 *uri.URL is less stuttering too, and more accurate.

*shrug*

[rsc, 2011-08-15 16:39:50]

yes but then uri.Parse, uri.Escape, etc are no longer correct, right?

[bradfitz, 2011-08-15 16:45:40]

On Mon, Aug 15, 2011 at 9:39 AM, Russ Cox <rsc@golang.org> wrote:

> yes but then uri.Parse, uri.Escape, etc are no longer correct, right?
>

yeah, suppose so.  could keep their original names.  still one character
shorter than http.ParseURL

[gustavo_niemeyer.net, 2011-08-15 17:01:21]

> It's unfortunate that naming the package url means
> you can't have a parameter or variable named url.
> It will make gofix difficult too.

url := urloc.Parse(...)

?

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-08-15 21:09:39]

I don't want to bikeshed this. "uri" is not the right name for this
package. (let's talk about queries.) "url" feels right to me. somehow
we cope with packages called time and rpc and path and image and utf8
and template and many others like that.

i'll accept a suggestion or two but i think you're focusing on the
wrong detail. the package name matters more than the variable names.

-rob

[bradfitz, 2011-08-15 21:11:33]

hg submit

On Mon, Aug 15, 2011 at 2:09 PM, Rob 'Commander' Pike <r@golang.org> wrote:

> I don't want to bikeshed this. "uri" is not the right name for this
> package. (let's talk about queries.) "url" feels right to me. somehow
> we cope with packages called time and rpc and path and image and utf8
> and template and many others like that.
>
> i'll accept a suggestion or two but i think you're focusing on the
> wrong detail. the package name matters more than the variable names.
>
> -rob
>

[rsc, 2011-08-15 21:18:41]

On Mon, Aug 15, 2011 at 17:09, Rob 'Commander' Pike <r@golang.org> wrote:
> I don't want to bikeshed this. "uri" is not the right name for this
> package. (let's talk about queries.) "url" feels right to me. somehow
> we cope with packages called time and rpc and path and image and utf8
> and template and many others like that.

that's all true.  i think url is the right name.

please don't submit this, though, until we
have gofix support for fixing people's code.
this is going to be a disruptive change and
is exactly the kind of tedious thing gofix
is for.

gofix can already insert new imports, which
is half the battle.  i think the other half is that
when you decide to insert an import of "url"
you first have to make a pass over the code
renaming all variables named url to url_
or something like that.

russ

[r, 2011-08-15 21:25:32]

i'll do gofix.

it's delightful that this is all in aid of fixing the template
package. sometimes i wonder how any code ever works.  however, i must
say that this pretty big change was really easy, much easier than in
other worlds i could name. clean dependency management is your friend
and fast compilation doesn't hurt.

speaking of which, david and i spent an hour understanding the
dependency cycle that make detected. its error reporting was
completely (i mean that) uninformative. it complained about
net/textproto and something else; the problem was go/doc and
runtime/cgo. it said something like, "cycle dropped: x <- y".  well, x
and y were not in a cycle we could uncover and anyway every dependency
cycle i've seen has more than one arrow.

so in case this comes up again, anyone with insight in debugging such
things should say something, please.

-rob

[rsc, 2011-08-15 22:15:01]

> speaking of which, david and i spent an hour understanding the
> dependency cycle that make detected. its error reporting was
> completely (i mean that) uninformative.

yes, make is like that.
when we replace make things will get better.

if you had managed to introduce the dependency
using ordinary builds (for example, by doing an all.bash
before the cycle, and then finding the cycle during a
gotest in exp/template), you'd have gotten a much
better error from 6l.

russ

[r, 2011-08-16 04:53:57]

Hello golang-dev@googlegroups.com, dsymonds@golang.org, bradfitz@golang.org,
rsc@golang.org, gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),

Please take another look.

[r2, 2011-08-16 04:55:29]

gofix module added

[dsymonds, 2011-08-16 06:36:28]

LGTM

http://codereview.appspot.com/4893043/diff/7003/src/cmd/godoc/main.go
File src/cmd/godoc/main.go (right):

http://codereview.appspot.com/4893043/diff/7003/src/cmd/godoc/main.go#newcode180
src/cmd/godoc/main.go:180: url_ := "http://" + addr + search
did you mean to hand-fix these, or leave them with the underscore?

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url.go
File src/cmd/gofix/url.go (right):

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url.go#newcode89
src/cmd/gofix/url.go:89: // Rename http.ParseURL to url.Parse
comment dreg

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url_test.go
File src/cmd/gofix/url_test.go (right):

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url_test.go#new...
src/cmd/gofix/url_test.go:8: addTestCases(urlTests)
any worry about code like this?
  type url string
  func (u url) esc() string { return http.QueryEscape(u) }

http://codereview.appspot.com/4893043/diff/7003/src/pkg/exp/template/funcs.go
File src/pkg/exp/template/funcs.go (right):

http://codereview.appspot.com/4893043/diff/7003/src/pkg/exp/template/funcs.go...
src/pkg/exp/template/funcs.go:10: 
rogue blank line

http://codereview.appspot.com/4893043/diff/7003/src/pkg/websocket/websocket_t...
File src/pkg/websocket/websocket_test.go (right):

http://codereview.appspot.com/4893043/diff/7003/src/pkg/websocket/websocket_t...
src/pkg/websocket/websocket_test.go:161: t.Errorf("Get: not URLError %#v", err)
s/URLError/url.Error/

[rsc, 2011-08-16 19:45:50]

http://codereview.appspot.com/4893043/diff/1/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/websocket/client.go#newc...
src/pkg/websocket/client.go:103: func Dial(uri, protocol, origin string) (ws
*Conn, err os.Error) {
Please change this back to url.
I want to avoid 'uri' in public APIs: it's just pedantry.

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url.go
File src/cmd/gofix/url.go (right):

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url.go#newcode44
src/cmd/gofix/url.go:44: 
If something imports "http" and mentions urls
without needing to import "url", it would be good
not to introduce the renames.

urlwalk := func(n interface{}) {
    // Is it an identifier?
    ...
}

fixing() := func() {
    if fixed {
        return
    }
    walk(f, urlwalk)
    addImport(f, "url")
    fixed = true
}

walk(f, func...
    s/fixed = true/fixing()/
})

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url.go#newcode65
src/cmd/gofix/url.go:65: // TODO: why does gofix not walk the Names in a
ValueSpec?
Just a bug.  Feel free to fix walk.

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url.go#newcode87
src/cmd/gofix/url.go:87: httpUsed := false
if !usesImport(f, "http") {
    deleteImport(f, "http")
}

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url.go#newcode89
src/cmd/gofix/url.go:89: // Rename http.ParseURL to url.Parse
On 2011/08/16 06:36:28, dsymonds wrote:
> comment dreg

Done.

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url_test.go
File src/cmd/gofix/url_test.go (right):

http://codereview.appspot.com/4893043/diff/7003/src/cmd/gofix/url_test.go#new...
src/cmd/gofix/url_test.go:8: addTestCases(urlTests)
On 2011/08/16 06:36:28, dsymonds wrote:
> any worry about code like this?
>   type url string
>   func (u url) esc() string { return http.QueryEscape(u) }

Won't that just work?

[rsc, 2011-08-16 19:46:33]

http://codereview.appspot.com/4893043/diff/1/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/4893043/diff/1/src/pkg/websocket/client.go#newc...
src/pkg/websocket/client.go:103: func Dial(uri, protocol, origin string) (ws
*Conn, err os.Error) {
On 2011/08/16 19:45:50, rsc wrote:
> Please change this back to url.
> I want to avoid 'uri' in public APIs: it's just pedantry.

Sorry, this is an ancient comment from
before I understood what was going on.  It was not
supposed to go out.

[r, 2011-08-17 02:44:58]

Hello dsymonds@golang.org, bradfitz@golang.org, rsc@golang.org,
gustavo@niemeyer.net, r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-08-17 03:12:25]

LGTM assuming new test passes

http://codereview.appspot.com/4893043/diff/6004/src/cmd/gofix/url_test.go
File src/cmd/gofix/url_test.go (right):

http://codereview.appspot.com/4893043/diff/6004/src/cmd/gofix/url_test.go#new...
src/cmd/gofix/url_test.go:93: }
On second thought I agree with David, because
the other parameters were a special case.
Please add

func (url *T) m() string {
    return url
}

[r, 2011-08-17 03:36:12]

*** Submitted as http://code.google.com/p/go/source/detail?r=f9399b55899e ***

url: new package
This is just moving the URL code from package http into its own package,
which has been planned for a while.
Besides clarity, this also breaks a nascent dependency cycle the new template
package was about to introduce.

Add a gofix module, url, and use it to generate changes outside http and url.

Sadness about the churn, gladness about some of the naming improvements.

R=dsymonds, bradfitz, rsc, gustavo, r
CC=golang-dev
http://codereview.appspot.com/4893043