Stage 1 conversion to JSON for storing Credentials.

[jbeda_google, 2011-09-13 18:29:37]

FYI -- some comments for your consideration :)

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py#newcod...
oauth2client/client.py:118: string, a JSON representation of this instance,
suitable to pass to
It is really strange that to_json returns a string but (from looking at the
derived classes) from_json takes a dict.  Personally, I'd rather they both take
a dict, but both taking a string is cool too.

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py#newcod...
oauth2client/client.py:124: def _from_json(cls, data):
I don't see where this is called in this module.   Should this be 'private' with
the underscore?  Perhaps rename to 'new_from_json'?

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py#newcod...
oauth2client/client.py:280: if 'token_expiry' in data:
Perhaps check to see if it isn't already a datatime.datetime?

http://codereview.appspot.com/4972065/diff/2001/oauth2client/multistore_file.py
File oauth2client/multistore_file.py (right):

http://codereview.appspot.com/4972065/diff/2001/oauth2client/multistore_file....
oauth2client/multistore_file.py:306: credential =
pickle.loads(base64.b64decode(cred_entry['credential']))
Don't keep this on my behalf.  I'm cool with ripping the bandaid off.

[jcgregorio_google, 2011-09-13 20:53:16]

Thanks, all the issues you've commented on are fixed. If you'd like to do a full
review I'd appreciate it! Thanks, -joe

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py#newcod...
oauth2client/client.py:118: string, a JSON representation of this instance,
suitable to pass to
On 2011/09/13 18:29:37, jbeda wrote:
> It is really strange that to_json returns a string but (from looking at the
> derived classes) from_json takes a dict.  Personally, I'd rather they both
take
> a dict, but both taking a string is cool too.

Done.

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py#newcod...
oauth2client/client.py:124: def _from_json(cls, data):
On 2011/09/13 18:29:37, jbeda wrote:
> I don't see where this is called in this module.   Should this be 'private'
with
> the underscore?  Perhaps rename to 'new_from_json'?

Done.

http://codereview.appspot.com/4972065/diff/2001/oauth2client/client.py#newcod...
oauth2client/client.py:280: if 'token_expiry' in data:
On 2011/09/13 18:29:37, jbeda wrote:
> Perhaps check to see if it isn't already a datatime.datetime?

Done.

http://codereview.appspot.com/4972065/diff/2001/oauth2client/multistore_file.py
File oauth2client/multistore_file.py (right):

http://codereview.appspot.com/4972065/diff/2001/oauth2client/multistore_file....
oauth2client/multistore_file.py:306: credential =
pickle.loads(base64.b64decode(cred_entry['credential']))
On 2011/09/13 18:29:37, jbeda wrote:
> Don't keep this on my behalf.  I'm cool with ripping the bandaid off.

Done.

[jbeda_google, 2011-09-14 17:52:11]

Looks good to me!  I'm excited to see this go in.

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py#newcode47
oauth2client/client.py:47: EXPIRY_FORMAT = "%Y-%m-%dT%H:%M:%S.%f"
Would it perhaps be better to make this be RFC3339 UTC?  Do we need the
microseconds?  At the very least specify that this is UTC?

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py#newcod...
oauth2client/client.py:268: return self._to_json(['store'])
Perhaps put this list in a class constant so that derived classes that want to
strip out more stuff can be sure to exclude any new non-persisted members in the
future?

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py#newcod...
oauth2client/client.py:271: def from_json(cls, data):
You say above that from_json should take a string but here it takes a dict.

http://codereview.appspot.com/4972065/diff/8001/samples/buzz/buzz.py
File samples/buzz/buzz.py (right):

http://codereview.appspot.com/4972065/diff/8001/samples/buzz/buzz.py#newcode91
samples/buzz/buzz.py:91: #import pdb; pdb.set_trace()
left over?

http://codereview.appspot.com/4972065/diff/8001/tests/test_oauth2client_file.py
File tests/test_oauth2client_file.py (right):

http://codereview.appspot.com/4972065/diff/8001/tests/test_oauth2client_file....
tests/test_oauth2client_file.py:120: def
test_multistore_non_existent_file(self):
Thanks!  I should have added tests in my original change for this.

[jcgregorio_google, 2011-09-14 20:46:55]

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py#newcode47
oauth2client/client.py:47: EXPIRY_FORMAT = "%Y-%m-%dT%H:%M:%S.%f"
On 2011/09/14 17:52:11, jbeda wrote:
> Would it perhaps be better to make this be RFC3339 UTC?  Do we need the
> microseconds?  At the very least specify that this is UTC?

Done.

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py#newcod...
oauth2client/client.py:268: return self._to_json(['store'])
On 2011/09/14 17:52:11, jbeda wrote:
> Perhaps put this list in a class constant so that derived classes that want to
> strip out more stuff can be sure to exclude any new non-persisted members in
the
> future?

Done.

http://codereview.appspot.com/4972065/diff/8001/oauth2client/client.py#newcod...
oauth2client/client.py:271: def from_json(cls, data):
On 2011/09/14 17:52:11, jbeda wrote:
> You say above that from_json should take a string but here it takes a dict.

Done.

http://codereview.appspot.com/4972065/diff/8001/samples/buzz/buzz.py
File samples/buzz/buzz.py (right):

http://codereview.appspot.com/4972065/diff/8001/samples/buzz/buzz.py#newcode91
samples/buzz/buzz.py:91: #import pdb; pdb.set_trace()
On 2011/09/14 17:52:11, jbeda wrote:
> left over?

Done.

[jbeda_google, 2011-09-14 21:37:42]

Looks good to me.  Thanks for taking the feedback!