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

proposal: net/http: add scheme constants #40587

Open
ainar-g opened this issue Aug 5, 2020 · 8 comments
Open

proposal: net/http: add scheme constants #40587

ainar-g opened this issue Aug 5, 2020 · 8 comments
Labels
Projects
Milestone

Comments

@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Aug 5, 2020

Over the years I've noticed quite a lot of “magic” strings in HTTP-related code, primarily "http" and "https". There are a lot of examples in net/http/... as well as in cmd/....

So, I propose to add these constants to net/http (or, perhaps, net/url?):

const (
	Scheme = "http"
	Secure = "https"
)

Example usage:

var u = &url.URL{
	Scheme: http.Scheme,
	Host:   host,
	Path:   path.Join("users", userID),
}
if u.Scheme != http.Secure {
	return errors.New("insecure api")
}
@gopherbot gopherbot added this to the Proposal milestone Aug 5, 2020
@gopherbot gopherbot added the Proposal label Aug 5, 2020
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Aug 5, 2020

I think Scheme and Secure are not obvious to the reader, if anything I would prefer plain HTTP and HTTPS though it stutters

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 5, 2020
@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Aug 15, 2020

@seankhliao My reasoning was similar to that of sort.Interface. Then again, we do have log.Log, so I don't really have a strong opinion about the exact naming.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 15, 2020

I don’t think these constants add anything for the reader. The literals http and https are descriptive, we’ll know, and unlikely to change at this point. A constant which represented these literals would be identical, except would be capitalised, which is misleading as HTTP schemes are not capitalised.

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Aug 15, 2020

@davecheney My main concern here are possible misspellings. I've already seen http being misspelled as htp in a private project. Luckily, the tests caught it, but still.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 20, 2020

In addition to the stutter, writing http.HTTP and http.HTTPS is 3 characters longer than "http" and "https".

People felt strongly about adding the HTTP method constants too but in practice they're not used because "GET" looks better than http.MethodGet. I feel like this would be the same.

As such, I'm not in favor of this.

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Aug 20, 2020

@bradfitz

[…] in practice they're not used because "GET" looks better than http.MethodGet. I feel like this would be the same.

That is not my experience, to be honest. Unless the code base is older than the named constants, I have almost always seen the constants preferred to literals.

As for the stuttering, my original proposal doesn't have that issue.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 21, 2020

As for the stuttering, my original proposal doesn't have that issue

Accepted, but the names http.Scheme and http.Secure don’t describe their contents; the former sounds like the name of a type and the latter sounds like some kind of boolean

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Sep 1, 2020

@davecheney I guess they could be http.SchemePlain and  http.SchemeSecure, just like we now have http.MethodGet and http.MethodPost. But that's way more typing.

I would hope that packages that implement other protocols would add their schemes in a similar fashion, so that we could have, say, ftp.Scheme and git.Scheme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.