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: make use of Method* constants internally #18577

Closed
mdlayher opened this issue Jan 9, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@mdlayher
Copy link
Member

commented Jan 9, 2017

Now that net/http has Method* constants, it'd be nice to get rid of all of the "GET", "POST, etc. string literals within the package itself. To me, this is the cleaner approach, and reduces the risk of a typo causing unexpected behavior later on.

This was prompted by my discussion with @dsnet here: https://go-review.googlesource.com/c/34981/#message-d7e7dafd6c2425712709da0b275efaf3140be26a.

I'd be happy to put in a CL for this for Go 1.9, if it's deemed worthwhile. I much prefer constants to string literals, but wanted to check in before going ahead with a CL.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

I rejected a change doing this in the past. It didn't make the code look better, and also didn't fix any bugs.

MethodGet isn't easier to read than "GET". In fact, the latter is more likely to catch bugs because MethodPost and MethodPut and MethodGet all look very similar, all starting with Method.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

It didn't make the code look better, and also didn't fix any bugs.

MethodGet isn't easier to read than "GET". In fact, the latter is more likely to catch bugs because MethodPost and MethodPut and MethodGet all look very similar, all starting with Method.

That rationale to prefer "GET", "PUT" over MethodGet, MethodPut sounds reasonable to me.

But it makes me curious what was the motivation to add the exported constants to net/http. If I'm finding the right CL/issue, there doesn't seem to much explanation for why they were added as exported symbols. From #12078:

It would also be useful to add the commonly used method constants to net/http from RFC 7231 along with PATCH from RFC 5789

From 0b314e1:

Fixes #12078

So if they're not useful in net/http, in what situation would it be a good idea to use them, if at all?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

Some people prefer them. I don't.

I didn't even realize that I didn't like them until I saw them used.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

If people would really prefer constants, I could live with:

const (
       mGET  = MethodGet
       mPOST = MethodPost
       mPUT  = MethodPut
)

And using those internally. But I don't like reading the "Method" stutter.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

... but that's kinda gross too.

@mdlayher

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2017

I'd be okay with doing the mGET approach if some consensus can be achieved.

The reason I brought this up is because I have frequently seen silly typos and such in repeated string literals, in projects outside the Go project itself. If it's never really been an issue in Go, maybe this is a good time for "if it ain't broke, don't fix it".

Some rudimentary grep-ing indicates that at least "HEAD" and "GET" are used pretty frequently, not counting uses in tests.

$ grep -r \"GET\" | grep -v "_test.go" | wc -l  
29
$ grep -r \"HEAD\" | grep -v "_test.go" | wc -l
24

@bradfitz bradfitz added this to the Go1.9Maybe milestone Jan 13, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

I'm going to close this since nobody feels strongly about it.

If somebody wants to send a change to use mFOO for Go 1.10, that's fine, but it doesn't need to be remain open.

@bradfitz bradfitz closed this Jun 7, 2017

@golang golang locked and limited conversation to collaborators Jun 7, 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.