code review 104480044: go.net/context: remove the Key type; replace it with st...

go.net/context: remove the Key type; replace it with interface{}.  This
makes the Context interface dependent only on standard packages, which
means types in other packages can implement this interface without
depending on go.net/context.

Remove the NewKey function and add examples showing how to use
unexported types to avoid key collisions.  This is the same model used
by http://www.gorillatoolkit.org/pkg/context, except we associate values
with a specific Context instead of storing them in a package-level map.

[Sameer Ajmani, 2014-07-02 02:00:28]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.net

[crawshaw, 2014-07-02 15:00:47]

LGTM

https://codereview.appspot.com/104480044/diff/1/context/context.go
File context/context.go (right):

https://codereview.appspot.com/104480044/diff/1/context/context.go#newcode47
context/context.go:47: //   // package foo defines a value that's stored in
Contexts.
tab indent for godoc

[dsymonds, 2014-07-03 01:28:58]

https://codereview.appspot.com/104480044/diff/1/context/context.go
File context/context.go (right):

https://codereview.appspot.com/104480044/diff/1/context/context.go#newcode36
context/context.go:36: // panics if the import path or name are empty, if name
contains ".", or if
why do we care if the name contains a period?

https://codereview.appspot.com/104480044/diff/1/context/context.go#newcode77
context/context.go:77: key := importPath + "." + name
oh, is this why? just use a different separator (e.g. a colon) that is not valid
in an import path, and then you don't have to restrict names.

https://codereview.appspot.com/104480044/diff/1/context/context.go#newcode79
context/context.go:79: panic(fmt.Sprintf("context.NewKey(%q, %q) called multiple
times", importPath, name))
This is presumably to stop people using context.NewKey twice, once in the
package that properly owns the, and once elsewhere. That doesn't seem like it'll
buy much, since that second place can trivially construct the key with a string
concatenation anyway.

https://codereview.appspot.com/104480044/diff/1/context/context_test.go
File context/context_test.go (right):

https://codereview.appspot.com/104480044/diff/1/context/context_test.go#newco...
context/context_test.go:252: var k1, k2 =
NewKey("code.google.com/p/go.net/context", "k1"),
NewKey("code.google.com/p/go.net/context", "k2")
it'd be easier to read if these were separate var decls.

[Sameer Ajmani, 2014-07-07 02:52:19]

The duplicate detection in NewKey is intended to help catch when two
different packages register the same key string.  Obviously this is easy to
circumvent; it's only intended to help prevent accidents.

Perhaps instead key should just be interface{}, with the restriction that
the value support =.  Then users can guarantee uniqueness by using a
pointer to a global var as their key.  A string is convenient, though.

As for the separator, I was intending for the import path plus key to look
like a fully qualified type name, hence the period.

I am wondering whether we should just drop this cl and leave things as is.
Your questions suggest that this change is not necessarily the
simplification I was going for.
On Jul 2, 2014 9:28 PM, <dsymonds@golang.org> wrote:

>
> https://codereview.appspot.com/104480044/diff/1/context/context.go
> File context/context.go (right):
>
> https://codereview.appspot.com/104480044/diff/1/context/
> context.go#newcode36
> context/context.go:36: // panics if the import path or name are empty,
> if name contains ".", or if
> why do we care if the name contains a period?
>
> https://codereview.appspot.com/104480044/diff/1/context/
> context.go#newcode77
> context/context.go:77: key := importPath + "." + name
> oh, is this why? just use a different separator (e.g. a colon) that is
> not valid in an import path, and then you don't have to restrict names.
>
> https://codereview.appspot.com/104480044/diff/1/context/
> context.go#newcode79
> context/context.go:79: panic(fmt.Sprintf("context.NewKey(%q, %q) called
> multiple times", importPath, name))
> This is presumably to stop people using context.NewKey twice, once in
> the package that properly owns the, and once elsewhere. That doesn't
> seem like it'll buy much, since that second place can trivially
> construct the key with a string concatenation anyway.
>
> https://codereview.appspot.com/104480044/diff/1/context/context_test.go
> File context/context_test.go (right):
>
> https://codereview.appspot.com/104480044/diff/1/context/
> context_test.go#newcode252
> context/context_test.go:252: var k1, k2 =
> NewKey("code.google.com/p/go.net/context", "k1"),
> NewKey("code.google.com/p/go.net/context", "k2")
> it'd be easier to read if these were separate var decls.
>
> https://codereview.appspot.com/104480044/
>

[Sameer Ajmani, 2014-07-10 14:20:22]

Substantial revision and simplification.

Keys are now interface{}, so users can define unexported types to avoid
collisions altogether.
This is the same technique as used in http://www.gorillatoolkit.org/pkg/context
Add tests that this actually works.

Add comments to the top of the file explaining idiomatic use of Contexts.



On 2014/07/07 02:52:19, Sameer Ajmani wrote:
> The duplicate detection in NewKey is intended to help catch when two
> different packages register the same key string.  Obviously this is easy to
> circumvent; it's only intended to help prevent accidents.
> 
> Perhaps instead key should just be interface{}, with the restriction that
> the value support =.  Then users can guarantee uniqueness by using a
> pointer to a global var as their key.  A string is convenient, though.
> 
> As for the separator, I was intending for the import path plus key to look
> like a fully qualified type name, hence the period.
> 
> I am wondering whether we should just drop this cl and leave things as is.
> Your questions suggest that this change is not necessarily the
> simplification I was going for.
> On Jul 2, 2014 9:28 PM, <mailto:dsymonds@golang.org> wrote:
> 
> >
> > https://codereview.appspot.com/104480044/diff/1/context/context.go
> > File context/context.go (right):
> >
> > https://codereview.appspot.com/104480044/diff/1/context/
> > context.go#newcode36
> > context/context.go:36: // panics if the import path or name are empty,
> > if name contains ".", or if
> > why do we care if the name contains a period?
> >
> > https://codereview.appspot.com/104480044/diff/1/context/
> > context.go#newcode77
> > context/context.go:77: key := importPath + "." + name
> > oh, is this why? just use a different separator (e.g. a colon) that is
> > not valid in an import path, and then you don't have to restrict names.
> >
> > https://codereview.appspot.com/104480044/diff/1/context/
> > context.go#newcode79
> > context/context.go:79: panic(fmt.Sprintf("context.NewKey(%q, %q) called
> > multiple times", importPath, name))
> > This is presumably to stop people using context.NewKey twice, once in
> > the package that properly owns the, and once elsewhere. That doesn't
> > seem like it'll buy much, since that second place can trivially
> > construct the key with a string concatenation anyway.
> >
> > https://codereview.appspot.com/104480044/diff/1/context/context_test.go
> > File context/context_test.go (right):
> >
> > https://codereview.appspot.com/104480044/diff/1/context/
> > context_test.go#newcode252
> > context/context_test.go:252: var k1, k2 =
> > NewKey("code.google.com/p/go.net/context", "k1"),
> > NewKey("code.google.com/p/go.net/context", "k2")
> > it'd be easier to read if these were separate var decls.
> >
> > https://codereview.appspot.com/104480044/
> >

[crawshaw, 2014-07-10 18:35:40]

LGTM

https://codereview.appspot.com/104480044/diff/20001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/104480044/diff/20001/context/context.go#newcode84
context/context.go:84: // Use context values only for request-scoped data that
transits
Perhaps this should be pulled into the list of don'ts in the package docs.

https://codereview.appspot.com/104480044/diff/20001/context/context.go#newcode97
context/context.go:97: // 	// package foo defines a value that's stored in
Contexts.
s/p/P/

https://codereview.appspot.com/104480044/diff/20001/context/context.go#newcode98
context/context.go:98: // 	package foo
A real-world example would be more compelling than a foo. Authentication is
common, how about package user?

[Sameer Ajmani, 2014-07-10 18:57:14]

https://codereview.appspot.com/104480044/diff/20001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/104480044/diff/20001/context/context.go#newcode84
context/context.go:84: // Use context values only for request-scoped data that
transits
On 2014/07/10 18:35:40, crawshaw wrote:
> Perhaps this should be pulled into the list of don'ts in the package docs.

Done.

https://codereview.appspot.com/104480044/diff/20001/context/context.go#newcode97
context/context.go:97: // 	// package foo defines a value that's stored in
Contexts.
On 2014/07/10 18:35:40, crawshaw wrote:
> s/p/P/

Done.

https://codereview.appspot.com/104480044/diff/20001/context/context.go#newcode98
context/context.go:98: // 	package foo
On 2014/07/10 18:35:40, crawshaw wrote:
> A real-world example would be more compelling than a foo. Authentication is
> common, how about package user?

Done.

[Sameer Ajmani, 2014-07-10 18:57:28]

*** Submitted as
https://code.google.com/p/go/source/detail?r=335bcad0b70a&repo=net ***

go.net/context: remove the Key type; replace it with interface{}.  This
makes the Context interface dependent only on standard packages, which
means types in other packages can implement this interface without
depending on go.net/context.

Remove the NewKey function and add examples showing how to use
unexported types to avoid key collisions.  This is the same model used
by http://www.gorillatoolkit.org/pkg/context, except we associate values
with a specific Context instead of storing them in a package-level map.

LGTM=crawshaw
R=golang-codereviews, crawshaw, dsymonds
CC=golang-codereviews, rsc
https://codereview.appspot.com/104480044