Google App Engine Django - Deserialize db.DateTimeProperty

[GvR, 2008-05-13 16:40:54]

Here are some comments.

http://codereview.appspot.com/950/diff/1/3
File appengine_django/serializer/python.py (right):

http://codereview.appspot.com/950/diff/1/3#newcode80
Line 80: TIME_FORMAT = "%H:%M:%S"
I think it makes more sense to define a single constant DATETIME_FORMAT (or at
least define that here too, as the concatenation of the other two).

http://codereview.appspot.com/950/diff/1/3#newcode130
Line 130: if isinstance(field, db.DateTimeProperty):
I'm reluctant to see a growing list of

  if isinstance(field, X): ...

blocks here.  I think it makes more sense to monkey-patch the Property
subclasses (with the eventual expectation that these changes will be adopted by
the appengine core).

Especially since DateProperty and TimeProperty will surely be next...

http://codereview.appspot.com/950/diff/1/3#newcode134
Line 134: field_value = d
Any reason you aren't writing

  field_value = datetime.datetime(*(t)[0:6])

?

http://codereview.appspot.com/950/diff/1/3#newcode135
Line 135: except (ValueError, TypeError), e:
I'm uncomfortable casting such a wide net.  I'd much rather only try errors in
the strptime() call, and guard the block with a typecheck on the value (checking
that it is a unicode instance).

http://codereview.appspot.com/950/diff/1/2
File appengine_django/serializer/xml.py (right):

http://codereview.appspot.com/950/diff/1/2#newcode142
Line 142: if isinstance(field, db.DateTimeProperty):
Why the duplicated code?  I guess this is another argument for monkey-patching
the class... :-)

[GvR, 2008-05-15 18:32:27]

Minor separate comments on this code.

http://codereview.appspot.com/950/diff/1/3
File appengine_django/serializer/python.py (right):

http://codereview.appspot.com/950/diff/1/3#newcode122
Line 122: if isinstance(field, db.Reference):
PS. Should use db.ReferenceProperty here -- db.Reference is deprecated.

http://codereview.appspot.com/950/diff/1/2
File appengine_django/serializer/xml.py (right):

http://codereview.appspot.com/950/diff/1/2#newcode129
Line 129: if isinstance(field, db.Reference):
PS. Should use db.ReferenceProperty here -- db.Reference is deprecated.