Change models.py to use typed class members instead of a list of strings.

Change models.py to use typed class members instead of a list of strings.

This strongly reduces the mismatch when deserializing and surprises that ensue
when a new member is added and is improperly initialized to None without warning.

This basically reproduces the same overall idea that protobuf/django/appengine
implements for serializable data but in a *much* lighter weight implementation.

R=csharp@chromium.org
BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169359

[M-A Ruel, 2012-11-23 17:04:19]

I also think that having the datastructure members be strongly typed makes it
easier to understand the code compared to the previous design.

I was able to get rid of the __persistent_module__ key because I finally learned
about cls.__subclasses__(), this is awesome. 

This CL gets rid of the ugly __new__ hack.

[csharp, 2012-11-23 18:31:57]

https://codereview.chromium.org/11414143/diff/2001/model.py
File model.py (right):

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode80
model.py:80: if issubclass(member_type, PersistentMixIn):
Link all these high level "if member_type something" so its clear at a glance
that only one will get executed

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode116
model.py:116: Warning: Not unit tested, use at your own risk!
Why not :)

https://codereview.chromium.org/11414143/diff/2001/verification/tree_status.py
File verification/tree_status.py (right):

https://codereview.chromium.org/11414143/diff/2001/verification/tree_status.p...
verification/tree_status.py:17: tree_status_url = str
Is it ok for this class to now get constructed without tree_status_url getting
explicitly set? It's ok to get set to just a default str?

This same question applies to a few of the other removed __init__ functions

[Peter Mayo, 2012-11-23 18:33:39]

https://codereview.chromium.org/11414143/diff/2001/model.py
File model.py (right):

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode6
model.py:6: to dict for serialization.
"to and from" is usual phrase, unless you mean to emphasize "from".

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode207
model.py:207: # cls.__persisten_members_cache on a class will implicitly set it
on its
__persisten*t*_members_cache

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode249
model.py:249: """Create a dictionary out of this object, e.g. Serialize the
object."""
e.g. -> i.e. ?

[M-A Ruel, 2012-11-23 18:44:06]

thanks, uploaded.

https://codereview.chromium.org/11414143/diff/2001/model.py
File model.py (right):

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode6
model.py:6: to dict for serialization.
On 2012/11/23 18:33:39, Peter Mayo wrote:
> "to and from" is usual phrase, unless you mean to emphasize "from".

done

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode80
model.py:80: if issubclass(member_type, PersistentMixIn):
On 2012/11/23 18:31:58, csharp wrote:
> Link all these high level "if member_type something" so its clear at a glance
> that only one will get executed

done and used elif to make it clearer.

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode116
model.py:116: Warning: Not unit tested, use at your own risk!
On 2012/11/23 18:31:58, csharp wrote:
> Why not :)

I only use it for debug output. If it becomes needed, the user will have to do
it himself.

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode207
model.py:207: # cls.__persisten_members_cache on a class will implicitly set it
on its
On 2012/11/23 18:33:39, Peter Mayo wrote:
> __persisten*t*_members_cache

done

https://codereview.chromium.org/11414143/diff/2001/model.py#newcode249
model.py:249: """Create a dictionary out of this object, e.g. Serialize the
object."""
On 2012/11/23 18:33:39, Peter Mayo wrote:
> e.g. -> i.e. ?

Right, done

https://codereview.chromium.org/11414143/diff/2001/verification/tree_status.py
File verification/tree_status.py (right):

https://codereview.chromium.org/11414143/diff/2001/verification/tree_status.p...
verification/tree_status.py:17: tree_status_url = str
On 2012/11/23 18:31:58, csharp wrote:
> Is it ok for this class to now get constructed without tree_status_url getting
> explicitly set? It's ok to get set to just a default str?
> 
> This same question applies to a few of the other removed __init__ functions

First, some background: it's a design issue that this instance doesn't know
about its parent, TreeStatusVerifier, which does know about the tree status url.
Since it doesn't know about the parent, it can't fetch the value. That's why I
copy it here too. It's not optimal from a design standpoint but it gets the job
done.

You are right that it must be non-empty to be of any use but I did not want to
add features like required=True in models.py. I left it out since I want to keep
the code in models.py as simple as possible. Adding member attributes likes
required=True would require usage of an object to represent the member type,
like how it's done in django[1] and AppEngine[2]. The goal here is to be
lighter-weight than that but a bit more structured than it was before.

[1]
https://docs.djangoproject.com/en/dev/howto/custom-model-fields/#django.db.mo...
[2]
https://developers.google.com/appengine/docs/python/datastore/typesandpropert...

[csharp, 2012-11-23 18:51:24]

lgtm

[commit-bot: I haz the power, 2012-11-23 18:53:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/11414143/2002

[commit-bot: I haz the power, 2012-11-23 18:53:38]

Change committed as 169359