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/http: Redirect documentation refers to non-existent url identifier (it was renamed) #21077

Closed
dmitshur opened this issue Jul 18, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@dmitshur
Copy link
Member

commented Jul 18, 2017

A very cool new package url was recently added to the Go standard library, see commit 1d8f822. To avoid a name collision, the existing url parameter of http.Redirect function was renamed to urlStr.

 // Redirect replies to the request with a redirect to url,
 // which may be a path relative to the request path.
-func Redirect(w ResponseWriter, r *Request, url string, code int) {
+func Redirect(w ResponseWriter, r *Request, urlStr string, code int) {

However, it looks like you forgot to update the documentation accordingly. It should be changed as follows:

-// Redirect replies to the request with a redirect to url,
+// Redirect replies to the request with a redirect to urlStr,
 // which may be a path relative to the request path.

Otherwise, it's not clear which url is being referred to. One may inadvertently misinterpret it to mean r.URL, which is not the case.

(I was going through my open tabs, and noticed I forgot to hit submit on this one. Better late than never. 🤷‍♂️)

@gopherbot

This comment has been minimized.

Copy link

commented Jul 18, 2017

CL https://golang.org/cl/49712 mentions this issue.

@gopherbot gopherbot closed this in 83fb9c8 Jul 18, 2017

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2017

CL 49712 implements the fix and is valid.

However, it got me thinking. Perhaps a better, alternative fix should be considered.

Go favors clarity of API documentation over implementation difficulties. See https://golang.org/s/style#named-result-parameters:

Clarity of docs is always more important than saving a line or two in your function.

The name urlStr was obviously chosen over url in response to the identifier collision (edit: see comment by @rsc at https://codereview.appspot.com/4893043#msg4). It is possible to avoid the identifier collision in an alternative way, by renaming the imported net/url package to something else (for example, import urlpkg "net/url"), then the documentation of net/http package can be more clear.

I think this would read better:

// Redirect replies to the request with a redirect to url,
// which may be a path relative to the request path.
// ...
func Redirect(w ResponseWriter, r *Request, url string, code int) {

Would it not? It's clear that url is a string from the signature, naming it urlStr does not improve external documentation clarity.

Renaming url import inside net/http may not be nice, but it only has to be in one .go file (where Redirect is defined), and it's an internal implementation detail.

It may not be worth it doing this, but I thought it was worth mentioning so it could be considered. /cc @bradfitz @ianlancetaylor @robpike

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

(The bot isn't mentioning this, likely because it's a closed issue.)

CL https://golang.org/cl/49930 mentions this issue.

gopherbot pushed a commit that referenced this issue Jul 20, 2017

net/http: improve signature of Redirect, NewRequest
In CL https://golang.org/cl/4893043 (6 years ago), a new package named
"url" was created (it is currently known as "net/url"). During that
change, some identifier name collisions were introduced, and two
parameters in net/http were renamed to "urlStr".

Since that time, Go has continued to put high emphasis on the quality
and readability of the documentation. Sometimes, that means making small
sacrifices in the implementation details of a package to ensure that
the godoc reads better, since that's what the majority of users interact
with. See https://golang.org/s/style#named-result-parameters:

> Clarity of docs is always more important than saving a line or two
> in your function.

I think the "urlStr" parameter name is suboptimal for godoc purposes,
and just "url" would be better.

During the review of https://golang.org/cl/4893043, it was also noted
by @rsc that having to rename parameters named "url" was suboptimal:

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

However, at the time, the name of the url package was still being
decided, and uri was an alternative name under consideration.
The reason urlStr was chosen is because it was a lesser evil
compared to naming the url package uri instead:

> 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.

> Please let's go with urlStr instead of uri.

Now that we have the Go 1 compatibility guarantee, the name of the
net/url package is fixed. However, it's possible to improve the
signature of Redirect, NewRequest functions in net/http package
for godoc purposes by creating a package global alias to url.Parse,
and renaming urlStr parameter to url in the exported funcs. This CL
does so.

Updates #21077.

Change-Id: Ibcc10e3825863a663e6ad91b6eb47b1862a299a6
Reviewed-on: https://go-review.googlesource.com/49930
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators Jul 20, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.