Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(143)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 950: Google App Engine Django - Deserialize db.DateTimeProperty

Can't Edit
Can't Publish+Mail
Start Review
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 #

Created: 17 years, 8 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -6 lines) Patch
appengine_django/serializer/python.py View 4 chunks +17 lines, -3 lines 5 comments Download
appengine_django/serializer/xml.py View 3 chunks +18 lines, -3 lines 2 comments Download
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... :-)
Sign in to reply to this message.
GvR
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, ...
17 years, 8 months ago (2008年05月15日 18:32:27 UTC) #2
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.
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /