Add 'user friendly' datastore API.

Add 'user friendly' datastore API.

This is a thin veneer on top of datastore.RawInterface, and may be used
with any filters, any backend, and it may be used interchangably and
inderleved with datastore.RawInterface.

In particular, this adds a convention for embedding keys inside of
loadable/savable objects.

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

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

[Vadim Sh., 2015-07-29 16:21:43]

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

https://codereview.chromium.org/1259593005/diff/1/service/datastore/datastore...
service/datastore/datastore.go:69: err = innerErr
are you sure you want to return error(nil) instead of nil?..

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

https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
service/datastore/interface.go:13: //   - obj is an object as specified by the
proto argument of Run
consider passing a struct 

type PartialResult struct {
  Obj interface{}
  Cursor func() (...)
}

That way RunCB signatures everywhere will be simpler, e.g. no need to worry
about defining getCursor arg if you ain't going to use it. (And if some new
field is added, no need to fix all signatures everywhere).

https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
service/datastore/interface.go:27: //   - *S where S is a struct
struct tagged with meta-how-do-you-call-them tags? ("$id")?

Also consider renaming it to KeyForObject or something. NewKey and NewKeyObj
sound synonymous... (since obj is understood as "key object", not "key for
object").

https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
service/datastore/interface.go:38: DecodeKey(encoded string) (Key, error)
document?

https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
service/datastore/interface.go:43: // Run executes the given query, and calls
`cb` for each successfully item.
typos: successfully item

https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
service/datastore/interface.go:45: // proto is a prototype of the objects which
will be passed to the callback.
It won't be written to, right?

https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
service/datastore/interface.go:75: // If src resolves to an Incomplete key, Put
will write the
its for autogenerating ids?

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

https://codereview.chromium.org/1259593005/diff/1/service/datastore/multiarg....
service/datastore/multiarg.go:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
I miss templates :)

[iannucci, 2015-07-29 16:38:35]

On mobile, but I like the partial result (maybe SingleResult? QueryResult?
Result?) Idea. I think I'd need two structs though, one for raw and one for
regular (I'm trying to avoid type assertions for anything dealing with the
raw interfaces). Does that seem OK?

On Wed, Jul 29, 2015, 09:21  <vadimsh@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/datastore.go
> File service/datastore/datastore.go (right):
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/datastore...
> service/datastore/datastore.go:69
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/datastore...>:
> err = innerErr
> are you sure you want to return error(nil) instead of nil?..
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface.go
> File service/datastore/interface.go (right):
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
> service/datastore/interface.go:13
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...>:
> //   - obj is an object as specified
> by the proto argument of Run
> consider passing a struct
>
> type PartialResult struct {
>    Obj interface{}
>    Cursor func() (...)
> }
>
> That way RunCB signatures everywhere will be simpler, e.g. no need to
> worry about defining getCursor arg if you ain't going to use it. (And if
> some new field is added, no need to fix all signatures everywhere).
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
> service/datastore/interface.go:27
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...>:
> //   - *S where S is a struct
> struct tagged with meta-how-do-you-call-them tags? ("$id")?
>
> Also consider renaming it to KeyForObject or something. NewKey and
> NewKeyObj sound synonymous... (since obj is understood as "key object",
> not "key for object").
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
> service/datastore/interface.go:38
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...>:
> DecodeKey(encoded string) (Key,
> error)
> document?
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
> service/datastore/interface.go:43
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...>:
> // Run executes the given query, and
> calls `cb` for each successfully item.
> typos: successfully item
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
> service/datastore/interface.go:45
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...>:
> // proto is a prototype of the
> objects which will be passed to the callback.
> It won't be written to, right?
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...
> service/datastore/interface.go:75
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/interface...>:
> // If src resolves to an Incomplete
> key, Put will write the
> its for autogenerating ids?
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/multiarg.go
> File service/datastore/multiarg.go (right):
>
>
>
https://codereview.chromium.org/1259593005/diff/1/service/datastore/multiarg....
> service/datastore/multiarg.go:1
>
<https://codereview.chromium.org/1259593005/diff/1/service/datastore/multiarg....>:
> // Copyright 2015 The Chromium Authors.
> All rights reserved.
> I miss templates :)
>
> https://codereview.chromium.org/1259593005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[iannucci, 2015-08-03 03:56:32]

PTAL!

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/da...
File service/datastore/datastore.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/da...
service/datastore/datastore.go:69: err = innerErr
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> are you sure you want to return error(nil) instead of nil?..

pretty sure it's the same thing.  `innerErr := error(nil)` is the same as `var
innerErr error`.

I do this because innerErr may have been assigned in the callback above.

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/in...
File service/datastore/interface.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/in...
service/datastore/interface.go:13: //   - obj is an object as specified by the
proto argument of Run
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> consider passing a struct 
> 
> type PartialResult struct {
>   Obj interface{}
>   Cursor func() (...)
> }
> 
> That way RunCB signatures everywhere will be simpler, e.g. no need to worry
> about defining getCursor arg if you ain't going to use it. (And if some new
> field is added, no need to fix all signatures everywhere).

I tried this, but it's not really worth the extra type. The godocs are already
pretty noisy (though I have some ideas on how to fix them). If we ever add a new
thing (pretty sure we won't), we can definitely revisit the struct (or
interface) idea.

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/in...
service/datastore/interface.go:27: //   - *S where S is a struct
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> struct tagged with meta-how-do-you-call-them tags? ("$id")?
> 
> Also consider renaming it to KeyForObject or something. NewKey and NewKeyObj
> sound synonymous... (since obj is understood as "key object", not "key for
> object").

Done.

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/in...
service/datastore/interface.go:38: DecodeKey(encoded string) (Key, error)
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> document?

Done.

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/in...
service/datastore/interface.go:43: // Run executes the given query, and calls
`cb` for each successfully item.
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> typos: successfully item

Done.

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/in...
service/datastore/interface.go:45: // proto is a prototype of the objects which
will be passed to the callback.
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> It won't be written to, right?

correct. documented.

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/in...
service/datastore/interface.go:75: // If src resolves to an Incomplete key, Put
will write the
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> its for autogenerating ids?

right. Incomplete specifically means `StringID == "" && IntID == 0`

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/mu...
File service/datastore/multiarg.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/1/service/datastore/mu...
service/datastore/multiarg.go:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2015/07/29 16:21:43, Vadim Sh. wrote:
> I miss templates :)

Yeah me too :)

[iannucci, 2015-08-03 04:00:34]

https://chromiumcodereview.appspot.com/1259593005/diff/80001/service/datastor...
File service/datastore/checkfilter_test.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/80001/service/datastor...
service/datastore/checkfilter_test.go:21: func TestCheckFilter(t *testing.T) {
this test probably should have been in the previous CL... I thought I had tested
the checkfilter code, but I guess I hadn't tested it in THIS package (and so it
had no coverage)

https://chromiumcodereview.appspot.com/1259593005/diff/80001/service/datastor...
File service/datastore/datastore_test.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/80001/service/datastor...
service/datastore/datastore_test.go:19: func fakeDatastoreFactory(c
context.Context) RawInterface {
This file is basically completely different now. Don't even look at the left
side. This tests all the new 'user friendly' interface code as well as all the
multitype.go code

https://chromiumcodereview.appspot.com/1259593005/diff/80001/service/datastor...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/80001/service/datastor...
service/datastore/pls.go:11: // GetPLS resolves obj into a PropertyLoadSaver.
Decided to give this a proper docstring

[dnj (Google), 2015-08-03 18:00:17]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-08-03 18:00:18]

Just some nits. Are there specific files that we should focus on for this
review? With all the add/delete, I find myself probably over-reviewing :)

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
File service/datastore/checkfilter.go (right):

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
service/datastore/checkfilter.go:44: return fmt.Errorf("datastore: GetMulti
callback is nil")
Use errors.New().

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
service/datastore/checkfilter.go:69: return fmt.Errorf("datastore: PutMulti
callback is nil")
Use errors.New().

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
service/datastore/checkfilter.go:101: return fmt.Errorf("datastore: DeleteMulti
callback is nil")
Use errors.New().

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
File service/datastore/context.go (right):

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
service/datastore/context.go:53: // SetRawFactory sets the function to produce
Datastore instances, as returned by
80-character wrapping for comments.

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
service/datastore/context.go:55: func SetRawFactory(c context.Context, rdsf
RawFactory) context.Context {
rdsfrdsfrdsfrdsfrdsf

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
service/datastore/context.go:59: // SetRaw sets the current Datastore object in
the context. Useful for testing with
80 character wrapping for comments.

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
service/datastore/context.go:67: curFiltsI := c.Value(rawDatastoreFilterKey)
Why not:
if curFiltsI, ok := c.Value(...).([]RawFilter); ok && curFiltsI != nil {
  return curFiltsI
}
return nil

This is more standard for a get+type assertion.

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
service/datastore/context.go:81: newFilts = append(newFilts,
getCurFilters(c)...)
Use "cur" here, instead of calling getCurFilters again.

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/inte...
File service/datastore/interface.go (right):

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/inte...
service/datastore/interface.go:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
80-character comment wrapping for this file.

https://codereview.chromium.org/1259593005/diff/100001/service/datastore/inte...
service/datastore/interface.go:78: RunInTransaction(f func(c context.Context)
error, opts *TransactionOptions) error
Maybe make this not a pointer, so we copy the options. This makes it clear that
the options value is not shared between caller and callee during the
transaction.

[iannucci, 2015-08-03 18:07:59]

On 2015/08/03 18:00:18, dnj (Google) wrote:
> Just some nits. Are there specific files that we should focus on for this
> review? With all the add/delete, I find myself probably over-reviewing :)
> 

Let me see if I can split this CL in half.

>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
> File service/datastore/checkfilter.go (right):
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
> service/datastore/checkfilter.go:44: return fmt.Errorf("datastore: GetMulti
> callback is nil")
> Use errors.New().
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
> service/datastore/checkfilter.go:69: return fmt.Errorf("datastore: PutMulti
> callback is nil")
> Use errors.New().
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/chec...
> service/datastore/checkfilter.go:101: return fmt.Errorf("datastore:
DeleteMulti
> callback is nil")
> Use errors.New().
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
> File service/datastore/context.go (right):
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
> service/datastore/context.go:53: // SetRawFactory sets the function to produce
> Datastore instances, as returned by
> 80-character wrapping for comments.
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
> service/datastore/context.go:55: func SetRawFactory(c context.Context, rdsf
> RawFactory) context.Context {
> rdsfrdsfrdsfrdsfrdsf
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
> service/datastore/context.go:59: // SetRaw sets the current Datastore object
in
> the context. Useful for testing with
> 80 character wrapping for comments.
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
> service/datastore/context.go:67: curFiltsI := c.Value(rawDatastoreFilterKey)
> Why not:
> if curFiltsI, ok := c.Value(...).([]RawFilter); ok && curFiltsI != nil {
>   return curFiltsI
> }
> return nil
> 
> This is more standard for a get+type assertion.
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/cont...
> service/datastore/context.go:81: newFilts = append(newFilts,
> getCurFilters(c)...)
> Use "cur" here, instead of calling getCurFilters again.
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/inte...
> File service/datastore/interface.go (right):
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/inte...
> service/datastore/interface.go:1: // Copyright 2015 The Chromium Authors. All
> rights reserved.
> 80-character comment wrapping for this file.
> 
>
https://codereview.chromium.org/1259593005/diff/100001/service/datastore/inte...
> service/datastore/interface.go:78: RunInTransaction(f func(c context.Context)
> error, opts *TransactionOptions) error
> Maybe make this not a pointer, so we copy the options. This makes it clear
that
> the options value is not shared between caller and callee during the
> transaction.

[iannucci, 2015-08-03 20:21:17]

OK, PTAL @ https://codereview.chromium.org/1270063002, which removes a bunch of
diff noise from this CL.

[iannucci, 2015-08-03 21:05:06]

Dependency for this is landed (thanks Vadim!), this one is ready I think.

[Vadim Sh., 2015-08-03 21:25:13]

various documentation nits

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/data...
File service/datastore/datastore.go (right):

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/data...
service/datastore/datastore.go:113: if !isOkType(reflect.TypeOf(src)) {
so Put requires passing values by pointer? Does it makes sense to allow pass by
value (e.g. if key is already set)?

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
File service/datastore/interface.go (right):

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
service/datastore/interface.go:111: //   - *S where S is a struct
well, at least it's documented, so I don't mind

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
service/datastore/interface.go:112: //   - *P where *P is a concrete type
implementing PropertyLoadSaver
IIUC, Put() will accept P (without star) too?

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
service/datastore/interface.go:114: // If src resolves to an Incomplete key, Put
will write the
nit: what does it mean for a struct to "resolve to an Incomplete key"?

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/mult...
File service/datastore/multiarg.go (right):

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/mult...
service/datastore/multiarg.go:109: // to reflect.  ¯\_(ツ)_/¯
:)

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/pls.go
File service/datastore/pls.go (right):

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/pls....
service/datastore/pls.go:19: // property types), the the resulting
PropertyLoadSaver will have a non-nil
typo: the the

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/pls....
service/datastore/pls.go:37: //      datastore, even if it was an otherwise
indexable type. If fieldName is
nit: clarify that fields are indexed by default

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/pls....
service/datastore/pls.go:42: //      can be to control filter behavior, or to
store key data when using
typo: can be used to control?

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/pls....
service/datastore/pls.go:43: //      the Interface.KeyForObject* methods. The
supported field types are:
KeyForObj?

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/raw_...
File service/datastore/raw_interface.go (right):

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/raw_...
service/datastore/raw_interface.go:109: // Callback execues once per item, in
the order of itemss. Callback may not
typo: itemss

[iannucci, 2015-08-03 21:51:48]

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
File service/datastore/datastore.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/datastore.go:113: if !isOkType(reflect.TypeOf(src)) {
On 2015/08/03 21:25:12, Vadim Sh. wrote:
> so Put requires passing values by pointer? Does it makes sense to allow pass
by
> value (e.g. if key is already set)?

Yeah, it requires pass by pointer. It COULD make sense to pass by value if they
key is already set... but not really. Note that PutMulti also requires
pass-by-pointer (even in the []S type cases, the slice acts like a pointer-to-S)

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
File service/datastore/interface.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/interface.go:111: //   - *S where S is a struct
On 2015/08/03 21:25:12, Vadim Sh. wrote:
> well, at least it's documented, so I don't mind

heh, yeah I tried to make these as obvious as possible without a real language
syntax.

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/interface.go:112: //   - *P where *P is a concrete type
implementing PropertyLoadSaver
On 2015/08/03 21:25:12, Vadim Sh. wrote:
> IIUC, Put() will accept P (without star) too?

Not really. Since PropertyLoadSaver has mutation methods (like SetMeta and
Load), it doesn't really make sense to implement the interface with a
non-pointer receiver (like... you could, by having a struct with a
pointer-to-something, but it would be confusing). If we have a use case for
this, we could extend multiarg.go to support it pretty easily.

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/interface.go:114: // If src resolves to an Incomplete key, Put
will write the
On 2015/08/03 21:25:12, Vadim Sh. wrote:
> nit: what does it mean for a struct to "resolve to an Incomplete key"?

Via the key extraction process described on KeyForObj above. "Incomplete" means
that stringID and intID are both zero-value (i.e. "" and 0). I could change this
to say something like:

The key will be extracted from src via KeyForObj. If KeyIncomplete(extractedKey)
is true, then Put will write the resolved key back to src.

Is that clearer?

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
File service/datastore/pls.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/pls.go:19: // property types), the the resulting
PropertyLoadSaver will have a non-nil
On 2015/08/03 21:25:12, Vadim Sh. wrote:
> typo: the the

Done.

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/pls.go:37: //      datastore, even if it was an otherwise
indexable type. If fieldName is
On 2015/08/03 21:25:13, Vadim Sh. wrote:
> nit: clarify that fields are indexed by default

Done.

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/pls.go:42: //      can be to control filter behavior, or to
store key data when using
On 2015/08/03 21:25:13, Vadim Sh. wrote:
> typo: can be used to control?

Done.

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/pls.go:43: //      the Interface.KeyForObject* methods. The
supported field types are:
On 2015/08/03 21:25:13, Vadim Sh. wrote:
> KeyForObj?

yep, Done.

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
File service/datastore/raw_interface.go (right):

https://chromiumcodereview.appspot.com/1259593005/diff/140001/service/datasto...
service/datastore/raw_interface.go:109: // Callback execues once per item, in
the order of itemss. Callback may not
On 2015/08/03 21:25:13, Vadim Sh. wrote:
> typo: itemss

done (and fixed a lot of these docs to be clearer)

[Vadim Sh., 2015-08-03 22:04:52]

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
File service/datastore/interface.go (right):

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
service/datastore/interface.go:112: //   - *P where *P is a concrete type
implementing PropertyLoadSaver
On 2015/08/03 21:51:48, iannucci wrote:
> On 2015/08/03 21:25:12, Vadim Sh. wrote:
> > IIUC, Put() will accept P (without star) too?
> 
> Not really. Since PropertyLoadSaver has mutation methods (like SetMeta and
> Load), it doesn't really make sense to implement the interface with a
> non-pointer receiver (like... you could, by having a struct with a
> pointer-to-something, but it would be confusing). If we have a use case for
> this, we could extend multiarg.go to support it pretty easily.

I mean I agree it doesn't make sense, but just looking at Put() implementation
it's not obvious that it rejects non-pointer recievers. isOkType hits "if
v.Implements(typeOfPropertyLoadSaver) {" and returns True without verifying that
it's indeed writable thingy. (or maybe I misunderstood something, or the actual
check is deeper in the stack).

https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
service/datastore/interface.go:114: // If src resolves to an Incomplete key, Put
will write the
On 2015/08/03 21:51:48, iannucci wrote:
> On 2015/08/03 21:25:12, Vadim Sh. wrote:
> > nit: what does it mean for a struct to "resolve to an Incomplete key"?
> 
> Via the key extraction process described on KeyForObj above. "Incomplete"
means
> that stringID and intID are both zero-value (i.e. "" and 0). I could change
this
> to say something like:
> 
> The key will be extracted from src via KeyForObj. If
KeyIncomplete(extractedKey)
> is true, then Put will write the resolved key back to src.
> 
> Is that clearer?

yes, it's clearer. I mean I understand what it meant, just nitpicking odd choice
of verbs :P

[iannucci, 2015-08-03 22:10:54]

On 2015/08/03 22:04:52, Vadim Sh. wrote:
>
https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
> File service/datastore/interface.go (right):
> 
>
https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
> service/datastore/interface.go:112: //   - *P where *P is a concrete type
> implementing PropertyLoadSaver
> On 2015/08/03 21:51:48, iannucci wrote:
> > On 2015/08/03 21:25:12, Vadim Sh. wrote:
> > > IIUC, Put() will accept P (without star) too?
> > 
> > Not really. Since PropertyLoadSaver has mutation methods (like SetMeta and
> > Load), it doesn't really make sense to implement the interface with a
> > non-pointer receiver (like... you could, by having a struct with a
> > pointer-to-something, but it would be confusing). If we have a use case for
> > this, we could extend multiarg.go to support it pretty easily.
> 
> I mean I agree it doesn't make sense, but just looking at Put() implementation
> it's not obvious that it rejects non-pointer recievers. isOkType hits "if
> v.Implements(typeOfPropertyLoadSaver) {" and returns True without verifying
that
> it's indeed writable thingy. (or maybe I misunderstood something, or the
actual
> check is deeper in the stack).

Yeah, the actual check happens in PutMulti. This was just a way to do quick
rejection of obviously bad types to get a clearer error in most of they typo
cases.

> 
>
https://codereview.chromium.org/1259593005/diff/140001/service/datastore/inte...
> service/datastore/interface.go:114: // If src resolves to an Incomplete key,
Put
> will write the
> On 2015/08/03 21:51:48, iannucci wrote:
> > On 2015/08/03 21:25:12, Vadim Sh. wrote:
> > > nit: what does it mean for a struct to "resolve to an Incomplete key"?
> > 
> > Via the key extraction process described on KeyForObj above. "Incomplete"
> means
> > that stringID and intID are both zero-value (i.e. "" and 0). I could change
> this
> > to say something like:
> > 
> > The key will be extracted from src via KeyForObj. If
> KeyIncomplete(extractedKey)
> > is true, then Put will write the resolved key back to src.
> > 
> > Is that clearer?
> 
> yes, it's clearer. I mean I understand what it meant, just nitpicking odd
choice
> of verbs :P

[Vadim Sh., 2015-08-03 22:12:30]

well, I don't know what else to nitpick, so lgtm

[iannucci, 2015-08-03 22:13:24]

Committed patchset #10 (id:180001) manually as
bdbb12dd3adfde49f4c39a52f826fda31ba94cd2 (presubmit successful).