Get rid of awkward proto argument to Interface.Run

Get rid of awkward proto argument to Interface.Run

R=dnj@chromium.org, vadimsh@chromium.org, estaab@chromium.org, tandrii@chromium.org
BUG=

Committed: https://github.com/luci/gae/commit/d09746539bf0eb3f7187d0593a1a17062c57acd9

[dnj, 2015-08-07 20:21:02]

https://codereview.chromium.org/1279703003/diff/1/service/datastore/datastore.go
File service/datastore/datastore.go (right):

https://codereview.chromium.org/1279703003/diff/1/service/datastore/datastore...
service/datastore/datastore.go:33: return fmt.Errorf("cb has the wrong number of
args: %T", cbIface)
WDYT about bundling all of these objections together into a simple error
message:
"cb does not match the required callback signature: %T != func(interface{},
CursorCB) bool"?

Also since this is internal interface static typing failure, a panic would be
appropriate here.

[iannucci, 2015-08-07 20:21:26]

On 2015/08/07 20:12:40, iannucci wrote:

Pay attention to the test changes, which reflect the 'real life' difference.
Also note that you don't have to do the *Key thing anymore.

[iannucci, 2015-08-07 20:22:03]

https://codereview.chromium.org/1279703003/diff/1/service/datastore/datastore.go
File service/datastore/datastore.go (right):

https://codereview.chromium.org/1279703003/diff/1/service/datastore/datastore...
service/datastore/datastore.go:33: return fmt.Errorf("cb has the wrong number of
args: %T", cbIface)
On 2015/08/07 20:21:02, dnj wrote:
> WDYT about bundling all of these objections together into a simple error
> message:
> "cb does not match the required callback signature: %T != func(interface{},
> CursorCB) bool"?
> 
> Also since this is internal interface static typing failure, a panic would be
> appropriate here.

yeah, ok, SGTM

[Vadim Sh., 2015-08-07 20:23:05]

lgtm with nits

https://codereview.chromium.org/1279703003/diff/20001/service/datastore/datas...
File service/datastore/datastore.go (right):

https://codereview.chromium.org/1279703003/diff/20001/service/datastore/datas...
service/datastore/datastore.go:32: if cbTyp.NumIn() != 2 || cbTyp.NumOut() != 1
{
check that cbTyp is a function?

https://codereview.chromium.org/1279703003/diff/20001/service/datastore/datas...
service/datastore/datastore.go:44: if cbTyp.In(0) == typeOfKey {
nit: you can probably cache all this checks (using cbTyp as a cache key).
Probably an overkill?

[iannucci, 2015-08-07 20:37:50]

fixed ptal

https://codereview.chromium.org/1279703003/diff/20001/service/datastore/datas...
File service/datastore/datastore.go (right):

https://codereview.chromium.org/1279703003/diff/20001/service/datastore/datas...
service/datastore/datastore.go:32: if cbTyp.NumIn() != 2 || cbTyp.NumOut() != 1
{
On 2015/08/07 20:23:05, Vadim Sh. wrote:
> check that cbTyp is a function?

Done.

https://codereview.chromium.org/1279703003/diff/20001/service/datastore/datas...
service/datastore/datastore.go:44: if cbTyp.In(0) == typeOfKey {
On 2015/08/07 20:23:05, Vadim Sh. wrote:
> nit: you can probably cache all this checks (using cbTyp as a cache key).
> Probably an overkill?

Yeah, probably can. I suspect it's overkill though. I'll add a todo in case we
ever profile stuff to look here.

https://codereview.chromium.org/1279703003/diff/20001/service/datastore/datas...
service/datastore/datastore.go:44: if cbTyp.In(0) == typeOfKey {
On 2015/08/07 20:23:05, Vadim Sh. wrote:
> nit: you can probably cache all this checks (using cbTyp as a cache key).
> Probably an overkill?

Yeah, probably can. I suspect it's overkill though. I'll add a todo in case we
ever profile stuff to look here.

[dnj, 2015-08-07 20:44:28]

lgtm w/ nit

https://codereview.chromium.org/1279703003/diff/80001/service/datastore/datas...
File service/datastore/datastore_test.go (right):

https://codereview.chromium.org/1279703003/diff/80001/service/datastore/datas...
service/datastore/datastore_test.go:769: ds.Run(q, cb)
nit: It's not clear that "badTypeCheck" is checking for a panic.

IMO rename to "badCallbackTypePanics". Probably better to just use
"ShouldPanic":
So(func() { ds.Run(q, func(int, CurcsorCB) bool { return false }) },
ShouldPanic)

[iannucci, 2015-08-07 21:05:42]

Ok, renamed (though I don't like ShouldPanic, because it can swallow other sorts
of panics, and I want to check that it's only this vary particular panic).

[iannucci, 2015-08-07 21:05:50]

Committed patchset #8 (id:140001) manually as
d09746539bf0eb3f7187d0593a1a17062c57acd9 (presubmit successful).