|
|
|
Created:
17 years, 8 months ago by mattbrown.nz Modified:
16 years, 5 months ago Reviewers:
GvR Base URL:
http://google-app-engine-django.googlecode.com/svn/trunk/ Visibility:
Public. |
Patch Set 1 #
Total messages: 2
|
GvR
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 ...
|
17 years, 8 months ago (2008年05月13日 16:40:54 UTC) #1 | |||||||||||||||||||
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... :-)
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.