Clean up callback interfaces.

Clean up callback interfaces.

R=dnj@chromium.org, estaab@chromium.org, vadimsh@chromium.org
BUG=https://github.com/luci/gae/issues/17

Committed: https://github.com/luci/gae/commit/33bfd9ddeef9db51b2b35ec68af32459f33878fd

[iannucci, 2015-12-14 22:11:39]

On 2015/12/12 at 23:25:20, iannucci wrote:
> 

PTAL?

[dnj, 2015-12-14 22:35:03]

lgtm w/ comments/nits

https://codereview.chromium.org/1521823003/diff/1/impl/memory/gkvlite_iter.go
File impl/memory/gkvlite_iter.go (right):

https://codereview.chromium.org/1521823003/diff/1/impl/memory/gkvlite_iter.go...
impl/memory/gkvlite_iter.go:104: err = nil
WDYT about just "return nil" here and elsewhere this pattern is used? This looks
a little awkward.

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

https://codereview.chromium.org/1521823003/diff/1/service/datastore/datastore...
service/datastore/datastore.go:47: func runParseCallback(cbIface interface{})
(isKey, hasErr, hasCursorCB bool, mat multiArgType) {
My overall concern here is that now the only allusion to getting a cursor is in
the documentation. Before, if you didn't acknowledge the prospect for a cursor,
you would hit an error that made it very apparent.

Just a quick though, but what if we made the signature a fixed:
callback(context.Context, item) error

Then added a context method, ds.GetCursor(c), that could be called on the
callback's Context. I feel like this would be more consistent with the rest of
the DS API.

https://codereview.chromium.org/1521823003/diff/1/service/datastore/datastore...
service/datastore/datastore.go:108: var cb func(reflect.Value, CursorCB) error
Either:
(a) Organize this more hierarchically:
if hasErr {
  if hasCursorCB {

  } else {

  }
} else {
  if hasCursorCB {

  } else {

  }
}

...

or (2) Use a switch statement:
switch {
case hasErr && hasCursorCB:
(etc.)
}

https://codereview.chromium.org/1521823003/diff/1/symbols.go
File symbols.go (right):

https://codereview.chromium.org/1521823003/diff/1/symbols.go#newcode13
symbols.go:13: var Stop error = stopErr{}
Why not just errors.New("stop iteration")?

[iannucci, 2015-12-14 23:03:09]

https://codereview.chromium.org/1521823003/diff/1/impl/memory/gkvlite_iter.go
File impl/memory/gkvlite_iter.go (right):

https://codereview.chromium.org/1521823003/diff/1/impl/memory/gkvlite_iter.go...
impl/memory/gkvlite_iter.go:104: err = nil
On 2015/12/14 at 22:35:02, dnj wrote:
> WDYT about just "return nil" here and elsewhere this pattern is used? This
looks a little awkward.

done

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

https://codereview.chromium.org/1521823003/diff/1/service/datastore/datastore...
service/datastore/datastore.go:47: func runParseCallback(cbIface interface{})
(isKey, hasErr, hasCursorCB bool, mat multiArgType) {
On 2015/12/14 at 22:35:03, dnj wrote:
> My overall concern here is that now the only allusion to getting a cursor is
in the documentation. Before, if you didn't acknowledge the prospect for a
cursor, you would hit an error that made it very apparent.
> 
> Just a quick though, but what if we made the signature a fixed:
> callback(context.Context, item) error
> 
> Then added a context method, ds.GetCursor(c), that could be called on the
callback's Context. I feel like this would be more consistent with the rest of
the DS API.

Discussed offline. I'm not a fan of having a method (package or interface-level)
like GetCursor that only works sometimes. It also makes every call to Run
mandatorially larger (by having a `c context.Context` argument), even when many
(most?) uses don't need a cursor at all. Additionally, since the context is
(always) untyped, it may make things more confusing ("why do I have a new
context now? do I need to use that for all datastore operations? What about
other operations taking a context?").

https://codereview.chromium.org/1521823003/diff/1/service/datastore/datastore...
service/datastore/datastore.go:108: var cb func(reflect.Value, CursorCB) error
On 2015/12/14 at 22:35:03, dnj wrote:
> Either:
> (a) Organize this more hierarchically:
> if hasErr {
>   if hasCursorCB {
> 
>   } else {
> 
>   }
> } else {
>   if hasCursorCB {
> 
>   } else {
> 
>   }
> }
> 
> ...
> 
> or (2) Use a switch statement:
> switch {
> case hasErr && hasCursorCB:
> (etc.)
> }

switch! done!

https://codereview.chromium.org/1521823003/diff/1/symbols.go
File symbols.go (right):

https://codereview.chromium.org/1521823003/diff/1/symbols.go#newcode13
symbols.go:13: var Stop error = stopErr{}
On 2015/12/14 at 22:35:03, dnj wrote:
> Why not just errors.New("stop iteration")?

because then golint complains that it's not ErrStop >_<

[iannucci, 2015-12-14 23:04:27]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-12-14 23:04:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org
Link to the patchset: https://codereview.chromium.org/1521823003/#ps20001
(title: "fixins")

[commit-bot: I haz the power, 2015-12-14 23:04:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521823003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521823003/20001

[commit-bot: I haz the power, 2015-12-14 23:07:24]

Description was changed from

==========
Clean up callback interfaces.

R=dnj@chromium.org, estaab@chromium.org, vadimsh@chromium.org
BUG=https://github.com/luci/gae/issues/17
==========

to

==========
Clean up callback interfaces.

R=dnj@chromium.org, estaab@chromium.org, vadimsh@chromium.org
BUG=https://github.com/luci/gae/issues/17

Committed:
https://github.com/luci/gae/commit/33bfd9ddeef9db51b2b35ec68af32459f33878fd
==========

[commit-bot: I haz the power, 2015-12-14 23:07:25]

Committed patchset #2 (id:20001) as
https://github.com/luci/gae/commit/33bfd9ddeef9db51b2b35ec68af32459f33878fd