-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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/httputil: export function singleJoiningSlash #44290
Comments
This is certainly not obvious to me. Copying a small, highly-specialized function seems much better than adding a dependency. |
@neild thanks for your quick response! In all mentioned repositories (which I only copied the first 3 I found on google and there seem to be a lot more that copy this function and import that package), this package is already a dependency, so you would not introduce a new one. But I agree, I should have mentioned this condition. Regardless of the question if code copying of small functions is bad, I don't really see a reason why making this function publicly accessible would cause any harm, when it has a clear benefit for people that do not want to copy code and maintain it themselves. |
Every piece of exported API surface comes with a cost, both for the package exporting it and for every package importing it. The costs are less obvious on the users' side, but are real: Increased API surface makes a package more difficult to use, any change to a dependency has the potential to break the code depending on it, the existence of an exported function to do something may incorrectly imply that this is a thing that should be done. It is tempting to see an internal function in a package, see a use for it, and then say that the function should be exported because it is demonstrably useful. Following this approach causes package APIs to grow organically, not by design. Package APIs should grow because the design suggests it, not just because the code is there. |
@neild I definitely agree that exporting functions always bears a risk and should correspond to a packages design, however I think in this case the design of the The function |
I agree that the function has utility, but I disagree with "probably will never change". It's already been mentioned in several issues, and I can easily see how changing some corner cases (such as for empty strings, or similar) could be convenient for the package, but detrimental or impossible if it is exported. See #25710 as an example of some hidden complexity. |
@toothrot I see thanks, I was not aware of the discussions around it. I still feel somewhat unsatisfied with this, because I see very big open source projects copying and relying on this code (including me) without any knowledge about the corner cases and hidden complexity around it and they all have to find out about this themselves. So while I agree that exporting the function as is might be suboptimal, is there not another way to provide a well documented publicly accessible function for that functionality in this package? |
cc @ianlancetaylor as this seems like a Proposal. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes, the function singleJoiningSlash is not exported.
go/src/net/http/httputil/reverseproxy.go
Lines 102 to 112 in 1004a7c
What did you expect to see?
The function should be exported, since several open source projects have to copy it instead of using it directly, which is suboptimal if you already use net/http/httputil as dependency:
Openshift:
https://github.com/openshift/console/blob/master/pkg/proxy/proxy.go#L69
AWS Proxy:
https://github.com/acquia/aws-proxy/blob/master/proxy/reverseproxy.go
Chronograf:
https://github.com/influxdata/influxdb/blob/master/chronograf/server/proxy.go
The text was updated successfully, but these errors were encountered: