Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(139)

Issue 4893043: code review 4893043: url: new package (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 3 months ago by r
Modified:
7 months, 3 weeks ago
Reviewers:
dsymonds, rsc, trishabusch69
CC:
dsymonds, bradfitz, rsc, gustavo_niemeyer.net, r2, g
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 13

Patch Set 2 : diff -r a6bbe1b3fb7d https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r a6bbe1b3fb7d https://go.googlecode.com/hg/ #

Total comments: 10

Patch Set 4 : diff -r a8d309fd526f https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 5 : diff -r a8d309fd526f https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+556 lines, -282 lines) Patch
M misc/dashboard/builder/http.go View 2 chunks +2 lines, -1 line 0 comments Download
M src/cmd/godoc/main.go View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M src/cmd/gofix/Makefile View 1 1 chunk +1 line, -0 lines 0 comments Download
A src/cmd/gofix/url.go View 1 2 3 1 chunk +116 lines, -0 lines 0 comments Download
A src/cmd/gofix/url_test.go View 1 2 3 4 1 chunk +147 lines, -0 lines 0 comments Download
M src/pkg/Makefile View 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/exp/template/funcs.go View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/http/Makefile View 1 chunk +0 lines, -1 line 0 comments Download
M src/pkg/http/cgi/child.go View 3 chunks +3 lines, -2 lines 0 comments Download
M src/pkg/http/cgi/host.go View 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/http/client.go View 7 chunks +9 lines, -8 lines 0 comments Download
M src/pkg/http/client_test.go View 1 3 chunks +7 lines, -6 lines 0 comments Download
M src/pkg/http/fs_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M src/pkg/http/readrequest_test.go View 7 chunks +7 lines, -6 lines 0 comments Download
M src/pkg/http/request.go View 1 2 9 chunks +27 lines, -89 lines 0 comments Download
M src/pkg/http/request_test.go View 4 chunks +4 lines, -3 lines 0 comments Download
M src/pkg/http/requestwrite_test.go View 5 chunks +5 lines, -4 lines 0 comments Download
M src/pkg/http/reverseproxy.go View 2 chunks +2 lines, -1 line 0 comments Download
M src/pkg/http/reverseproxy_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M src/pkg/http/serve_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M src/pkg/http/server.go View 1 3 chunks +14 lines, -13 lines 0 comments Download
M src/pkg/http/transport.go View 1 2 7 chunks +13 lines, -12 lines 0 comments Download
M src/pkg/http/transport_test.go View 4 chunks +4 lines, -3 lines 0 comments Download
M src/pkg/url/Makefile View 1 chunk +1 line, -17 lines 0 comments Download
M src/pkg/url/url.go View 1 15 chunks +127 lines, -56 lines 0 comments Download
M src/pkg/url/url_test.go View 1 10 chunks +47 lines, -47 lines 0 comments Download
M src/pkg/websocket/client.go View 1 3 chunks +5 lines, -4 lines 0 comments Download
M src/pkg/websocket/websocket_test.go View 1 2 3 2 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 22
r
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
13 years, 3 months ago (2011-08-15 08:12:40 UTC) #1
dsymonds
LGTM It's good to see this finally done. Let's not get hung up on URI ...
13 years, 3 months ago (2011-08-15 09:20:23 UTC) #2
bradfitz
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 ...
13 years, 3 months ago (2011-08-15 15:02:18 UTC) #3
rsc
It's unfortunate that naming the package url means you can't have a parameter or variable ...
13 years, 3 months ago (2011-08-15 16:25:51 UTC) #4
bradfitz
On Mon, Aug 15, 2011 at 9:25 AM, Russ Cox <rsc@golang.org> wrote: > It's unfortunate ...
13 years, 3 months ago (2011-08-15 16:36:12 UTC) #5
rsc
yes but then uri.Parse, uri.Escape, etc are no longer correct, right?
13 years, 3 months ago (2011-08-15 16:39:50 UTC) #6
bradfitz
On Mon, Aug 15, 2011 at 9:39 AM, Russ Cox <rsc@golang.org> wrote: > yes but ...
13 years, 3 months ago (2011-08-15 16:45:40 UTC) #7
gustavo_niemeyer.net
> It's unfortunate that naming the package url means > you can't have a parameter ...
13 years, 3 months ago (2011-08-15 17:01:21 UTC) #8
r
I don't want to bikeshed this. "uri" is not the right name for this package. ...
13 years, 3 months ago (2011-08-15 21:09:39 UTC) #9
bradfitz
hg submit On Mon, Aug 15, 2011 at 2:09 PM, Rob 'Commander' Pike <r@golang.org> wrote: ...
13 years, 3 months ago (2011-08-15 21:11:33 UTC) #10
rsc
On Mon, Aug 15, 2011 at 17:09, Rob 'Commander' Pike <r@golang.org> wrote: > I don't ...
13 years, 3 months ago (2011-08-15 21:18:41 UTC) #11
r
i'll do gofix. it's delightful that this is all in aid of fixing the template ...
13 years, 3 months ago (2011-08-15 21:25:32 UTC) #12
rsc
> speaking of which, david and i spent an hour understanding the > dependency cycle ...
13 years, 3 months ago (2011-08-15 22:15:01 UTC) #13
r
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.
13 years, 3 months ago (2011-08-16 04:53:57 UTC) #14
r2
gofix module added
13 years, 3 months ago (2011-08-16 04:55:29 UTC) #15
dsymonds
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 ...
13 years, 3 months ago (2011-08-16 06:36:28 UTC) #16
rsc
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#newcode103 src/pkg/websocket/client.go:103: func Dial(uri, protocol, origin string) (ws *Conn, err os.Error) ...
13 years, 3 months ago (2011-08-16 19:45:50 UTC) #17
rsc
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#newcode103 src/pkg/websocket/client.go:103: func Dial(uri, protocol, origin string) (ws *Conn, err os.Error) ...
13 years, 3 months ago (2011-08-16 19:46:33 UTC) #18
r
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.
13 years, 3 months ago (2011-08-17 02:44:58 UTC) #19
rsc
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#newcode93 src/cmd/gofix/url_test.go:93: } On second thought ...
13 years, 3 months ago (2011-08-17 03:12:25 UTC) #20
r
*** Submitted as http://code.google.com/p/go/source/detail?r=f9399b55899e *** url: new package This is just moving the URL code ...
13 years, 3 months ago (2011-08-17 03:36:12 UTC) #21
trishabusch69
7 months, 3 weeks ago (2024-04-15 19:12:41 UTC) #22
Message was sent while issue was closed.

          
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b