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

Jump to bottom

context: WithValue documentation should discourage use of basic types #17302

Closed
mdlayher opened this issue Sep 30, 2016 · 7 comments
Closed

context: WithValue documentation should discourage use of basic types #17302

mdlayher opened this issue Sep 30, 2016 · 7 comments
Labels
Documentation FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Milestone
Go1.8

Comments

@mdlayher
Copy link
Member

mdlayher commented Sep 30, 2016 •
edited

Related to: #17293.

I've submitted a fix to golint to begin warning about this, but I also feel that it would be a good idea to explicitly document that only user-defined types should be used as keys in context.WithValue.

As of now, there is no such warning. The only advice given is:

The provided key must be comparable.

EDIT: I was incorrect, see: #17302 (comment).

CC @bradfitz @alandonovan
@bradfitz
Copy link
Contributor

Yes, please!

@bradfitz bradfitz added this to the Go1.8 milestone Sep 30, 2016
@bradfitz bradfitz added Suggested Issues that may be good for new contributors looking for work to do. Documentation labels Sep 30, 2016
@mdlayher
Copy link
Member Author

I'll submit a CL shortly.

@mdlayher
Copy link
Member Author

It appears I was incorrect: https://golang.org/pkg/context/#Context

        // A key identifies a specific value in a Context. Functions that wish
        // to store values in Context typically allocate a key in a global
        // variable then use that key as the argument to context.WithValue and
        // Context.Value. A key can be any type that supports equality;
        // packages should define keys as an unexported type to avoid
        // collisions.

Should this be reiterated in the documentation for context.WithValue, or is the documentation on the context.Context interface enough?

@bradfitz
Copy link
Contributor

More warnings SGTM.

@mdlayher
Copy link
Member Author

CL submitted: https://go-review.googlesource.com/c/30134/.

Happy to re-phrase as needed.

@gopherbot
Copy link

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

@bradfitz bradfitz mentioned this issue Nov 6, 2016
Closed
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Nov 12, 2016
@bradfitz
context: document appropriate WithValue key type more
c921d8f 
Fixes #17826
Updates #17302

Change-Id: I7c1ebd965e679e7169a97e62d27ae3ede2473aa1
Reviewed-on: https://go-review.googlesource.com/33152
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Nov 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
3 participants
@bradfitz @mdlayher @gopherbot