net/http: Redirect documentation refers to non-existent url identifier (it was renamed) #21077
Comments
|
CL https://golang.org/cl/49712 mentions this issue. |
|
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:
The name 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 Renaming It may not be worth it doing this, but I thought it was worth mentioning so it could be considered. /cc @bradfitz @ianlancetaylor @robpike |
|
(The bot isn't mentioning this, likely because it's a closed issue.) CL https://golang.org/cl/49930 mentions this issue. |
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>
A very cool new package
urlwas recently added to the Go standard library, see commit 1d8f822. To avoid a name collision, the existingurlparameter ofhttp.Redirectfunction was renamed tourlStr.However, it looks like you forgot to update the documentation accordingly. It should be changed as follows:
Otherwise, it's not clear which
urlis being referred to. One may inadvertently misinterpret it to meanr.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. 🤷♂️)
The text was updated successfully, but these errors were encountered: