Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(60)

Issue 4532086: Add makepatch() method. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 1 month ago by jcgregorio
Modified:
14 years, 1 month ago
Reviewers:
aiuto
CC:
google-api-python-client_googlegroups.com
Visibility:
Public.

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -0 lines) Patch
M apiclient/model.py View 1 chunk +39 lines, -0 lines 4 comments Download
A tests/test_model.py View 1 chunk +71 lines, -0 lines 0 comments Download

Messages

Total messages: 3
jcgregorio
14 years, 1 month ago (2011-05-26 18:53:09 UTC) #1
aiuto
lgtm The comments were all about (potential) speed over obvious correctness. Your call. http://codereview.appspot.com/4532086/diff/1/apiclient/model.py File ...
14 years, 1 month ago (2011-05-26 19:18:16 UTC) #2
jcgregorio
14 years, 1 month ago (2011-05-26 19:42:07 UTC) #3
http://codereview.appspot.com/4532086/diff/1/apiclient/model.py
File apiclient/model.py (right):

http://codereview.appspot.com/4532086/diff/1/apiclient/model.py#newcode327
apiclient/model.py:327: if key not in modified:
On 2011/05/26 19:18:16, aiuto wrote:
> Just a thought.
> It might be slightly faster as
> modified_value = modified.get(key)
> if not modified_value:
>   ..
> 
> and then  modified[key] => modified_value in the other 2 instances.
> 
> Also, maybe the outer loop as
>     for key,value in original.iteritems():

Done.

http://codereview.appspot.com/4532086/diff/1/apiclient/model.py#newcode328
apiclient/model.py:328: patch[key] = None
On 2011/05/26 19:18:16, aiuto wrote:
> It took me a few minutes to realize that this was indeed a delete of the
> element. Could you add some comments to indicate the effect of the various
> replacements.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b