DISCUSSION PR: Re-writing Entity to subclass object instead of dict.

[dhermes]

This fixes #396, but I am happy to close it if there are issues with it. (Hence the DISCUSSION in the description.)

It is a first-step towards the discussion in #336 with @elibixby and @proppy.

The Entity class doesn't attempt to be a dictionary, rather it provides __getitem__ (and set and del) semantics and update_properties and clear_properties methods for updating the properties and leaving the rest of the entity alone.

The long term goal is to provide a way to do something like:

>>> e1 = Entity('SomeKind', 1, prop1=u'foo', prop2=u'bar')
>>> e1
<Entity[{'kind': 'SomeKind', 'id': 1}] {'prop1': u'foo', 'prop2': u'bar'}>
>>> e2 = Entity('SomeKind', 1, {'prop1': u'foo', 'prop2': u'bar'})
>>> e2
<Entity[{'kind': 'SomeKind', 'id': 1}] {'prop1': u'foo', 'prop2': u'bar'}>
>>> e1 == e2
True
>>> e_partial = Entity('ParentKind', 1, 'ChildKind', prop=True)
>>> e_partial
<Entity[{'kind': 'Parent', 'id': 1}, {'kind': 'Child'}] {'prop': True}>

and I think to get there this is necessary to separate the property data (as _data) from the other metadata.

I am still trying to figure out a way to squeeze in "special" keyword arguments like

• key (may be possible to just implement a method like key.to_args() so we don't need a kwarg. Or instead we could check if len(positional_args) == 1 and isinstance(positional_args[0], Key))
• dataset (Implicit dataset from environ #444 may make this unnecessary if we take it a bit further and my design doc just says forget about it and set it on the key. Though the ID is still needed if we only have the key parts from positional_args.)
• exclude_from_indexes (as a power-user feature, it may not be necessary to put this in the constructor)

but that is outside the scope of this PR / discussion.

[coveralls]

Coverage remained the same when pulling c1137f6 on dhermes:do-not-inherit-dict into 21e5f45 on GoogleCloudPlatform:master.

[elibixby]

Give the discussion in #446, it seems that syntax should actually be Entity(Key('SomeKind',1), ...) in which case I think Entity(key, props) makes more sense than Entity(key, **props). This also allows for easy addition of other special kwargs (although as per the design doc I think the only necessary one is exclude_from_indexes).

[dhermes]

First, let me note that my trailing comments give a context for this PR, but don't actually pertain to the code in the PR.

---

To address @elibixby remarks:

Yes #444 needs to be implemented for

Entity('Kind', 1, prop1=u'foo')

to have a chance of working.

I think in addition to the constructors I mentioned above we should also enable

>>> key
<Key[{'kind': 'SomeKind', 'id': 1}]>
>>> Entity(key, prop1=u'foo', prop2=u'bar')
<Entity[{'kind': 'SomeKind', 'id': 1}] {'prop1': u'foo', 'prop2': u'bar'}>
>>> Entity(key, {'prop1': u'foo', 'prop2': u'bar'})
<Entity[{'kind': 'SomeKind', 'id': 1}] {'prop1': u'foo', 'prop2': u'bar'}>

This will allow for things like keys from multiple datasets in the same application and keys with namespaces.

[elibixby]

Is this providing too many ways to achieve the same thing? I do like the idea of being able to instantiate an entity with a path (and providing the key with the "default" dataset from the environment), but I'm not sure if Entity(key, dict(prop1=u'foo', prop2=u'bar')) is much more burdensome than Entity(key, prop1=u'foo', prop2=u'bar'). If both exist, then users must remember to pass a dict rather than kwargs when setting exclude_from_indexes (or any meta-properties that may crop up in the future)

Just a my $.02. I think the ideas have been fantastic so far, and am really excited with all the discussion.

[dhermes]

@elibixby You're probably right that we should only accept a key as a positional arg since the syntax would be the same.

RE: exclude_from_indexes I'm fine with

>>> e = Entity(key, prop1=u'foo', prop2=u'bar')
>>> e.remove_prop_from_index('prop1')

A full-blown ORM (like ndb) makes this easier when Propertys are objects:

class Foo(ndb.Model):
    bar = ndb.StringProperty(indexed=False)

[elibixby]

Yes! I like that!

[coveralls]

Changes Unknown when pulling 0d79df0 on dhermes:do-not-inherit-dict into * on GoogleCloudPlatform:master*.

[coveralls]

Coverage remained the same when pulling e43096a on dhermes:do-not-inherit-dict into b343acf on GoogleCloudPlatform:master.

[tseaver]

Overall, I don't see any advantage to the "object-with-a-dict" versus "object-derived-from-dict".

[dhermes]

@tseaver let's forget about this for now. I initially did it because I didn't want a split MRO on Entity but it won't matter in the long-run since Entity won't need a dataset.

In the interest of "only one way", the note above for me is the best option for a constructor. (First positional arg must be a key, if more than one, second must be a dict. Otherwise, kwargs acts as the dict.)

I'll rebase #456 to forget about this PR and we'll go from there (after wrapping up #444).

[dhermes]

Closing PR but leaving the branch alive in my fork so may revisit. It likely won't be necessary as Entity gets stripped down further.