Re: Loading of client_secrets JSON file backed by a cache (issue 6349087)

[al...@cloudware.it]

Actually, NDB doesn't pick up RPCs that were not originally registered
with its autobatcher. Plus, in this specific case I don't think async is
really worth it.

So, here's a simplified version, w/o async.

http://codereview.appspot.com/6349087/

[jcgre...@google.com]

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py#newcode122
oauth2client/clientsecrets.py:122: cache: a cache service client that
implements get() and set() methods.
missing 'filename'

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py#newcode141
oauth2client/clientsecrets.py:141: contents = { _CLIENT_TYPE:
client_type, _CLIENT_INFO: client_info }
Why not use the same format that's in the client_secrets.json file? I.e.

contents = { client_type: client_info }

It is also easy to pull apart:

client_type, client_info = obj.iteritems().next()

http://codereview.appspot.com/6349087/

[al...@cloudware.it]

Thanks!

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py#newcode122
oauth2client/clientsecrets.py:122: cache: a cache service client that
implements get() and set() methods.

On 2012/07/12 12:55:01, jcgregorio_google wrote:
> missing 'filename'

Done.

http://codereview.appspot.com/6349087/diff/2002/oauth2client/clientsecrets.py#newcode141
oauth2client/clientsecrets.py:141: contents = { _CLIENT_TYPE:
client_type, _CLIENT_INFO: client_info }

On 2012/07/12 12:55:01, jcgregorio_google wrote:
> Why not use the same format that's in the client_secrets.json file?
I.e.

> contents = { client_type: client_info }

> It is also easy to pull apart:

> client_type, client_info = obj.iteritems().next()

Done. Overthinking from the prev. implementation.

http://codereview.appspot.com/6349087/

[jcgre...@google.com]

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py#newcode111
oauth2client/clientsecrets.py:111: Tipical cache storage would be App
Engine memcache service,
Typical

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py#newcode122
oauth2client/clientsecrets.py:122: filename: path to a
client_secrets.json file on a filesystem
filename: string, Path to

In general the pattern is:

parameter-name: type, Complete sentence describing parameter.

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clientsecrets.py
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clientsecrets.py#newcode83
tests/test_oauth2client_clientsecrets.py:83: self.cache = {}
self.last_get_ns = None
self.last_set_ns = None

http://codereview.appspot.com/6349087/

[al...@cloudware.it]

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py#newcode111
oauth2client/clientsecrets.py:111: Tipical cache storage would be App
Engine memcache service,

On 2012/07/12 15:37:24, jcgregorio_google wrote:
> Typical

Done.

http://codereview.appspot.com/6349087/diff/7002/oauth2client/clientsecrets.py#newcode122
oauth2client/clientsecrets.py:122: filename: path to a
client_secrets.json file on a filesystem

On 2012/07/12 15:37:24, jcgregorio_google wrote:
> filename: string, Path to

> In general the pattern is:

> parameter-name: type, Complete sentence describing parameter.

Done.

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clientsecrets.py
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/7002/tests/test_oauth2client_clientsecrets.py#newcode83
tests/test_oauth2client_clientsecrets.py:83: self.cache = {}

On 2012/07/12 15:37:24, jcgregorio_google wrote:
> self.last_get_ns = None
> self.last_set_ns = None

Done.

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py#newcode108
oauth2client/clientsecrets.py:108: def load_cached(filename, cache):
Hey, I was thinking to change the signature to

def load_cached(filename, cache=None)

Where cache=None just falls back to loadfile(filename). This seems to
follow other libs (e.g. httplib2) closer. Plus, that way other pieces of
oauth2client could be alternated (where needed for e.g. App Engine) and
be totally backwards compatible, for instance:

flow_from_clientsecrets(filename, scope, message=None, cache=None)

or, OAuth2DecoratorFromClientSecrets:
def __init__(self, filename, scope, message=None, cache=None):

or, would it be better for the above __init__() code to just
load_cached() instead of (currenty) loadfile().

What do you think?

http://codereview.appspot.com/6349087/

[jcgre...@google.com]

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py#newcode108
oauth2client/clientsecrets.py:108: def load_cached(filename, cache):

Sounds good to me, rename this function to loadfile, rename the original
loadfile to _loadfile. You can also add the optional 'cache' arguments
to all the *FromClientSecrets classes and methods if you feel motivated
:)

[al...@cloudware.it]

The optional 'cache' param change for *FromClientSecrets is trivial but
I can add some test cases for client.py and appengine.py if needed.

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py
File oauth2client/clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/10001/oauth2client/clientsecrets.py#newcode108
oauth2client/clientsecrets.py:108: def load_cached(filename, cache):

On 2012/07/13 13:39:40, jcgregorio_google wrote:
> Sounds good to me, rename this function to loadfile, rename the
original
> loadfile to _loadfile. You can also add the optional 'cache' arguments
to all
> the *FromClientSecrets classes and methods if you feel motivated :)

> On 2012/07/12 18:37:10, crhyme wrote:
> > Hey, I was thinking to change the signature to
> >
> > def load_cached(filename, cache=None)
> >
> > Where cache=None just falls back to loadfile(filename). This seems
to follow
> > other libs (e.g. httplib2) closer. Plus, that way other pieces of
oauth2client
> > could be alternated (where needed for e.g. App Engine) and be
totally
> backwards
> > compatible, for instance:
> >
> > flow_from_clientsecrets(filename, scope, message=None, cache=None)
> >
> > or, OAuth2DecoratorFromClientSecrets:
> > def __init__(self, filename, scope, message=None, cache=None):
> >
> > or, would it be better for the above __init__() code to just
load_cached()
> > instead of (currenty) loadfile().
> >
> > What do you think?

Done.

http://codereview.appspot.com/6349087/

[jcgre...@google.com]

On 2012/07/13 14:50:51, crhyme wrote:
> The optional 'cache' param change for *FromClientSecrets is trivial
but I can
> add some test cases for client.py and appengine.py if needed.

Yes, please do.

[jcgre...@google.com]

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py#newcode457
oauth2client/appengine.py:457: cache: an optional cache service client

that implements get() and set()

An

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clientsecrets.py
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clientsecrets.py#newcode73
tests/test_oauth2client_clientsecrets.py:73: 'afilethatisntthere.json'))
Looks like there are multiple tests that need a filename guaranteed not
to exist. Break out as a module level constant.

http://codereview.appspot.com/6349087/

[al...@cloudware.it]

Hopefully I got this right.

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6349087/diff/11002/oauth2client/appengine.py#newcode457
oauth2client/appengine.py:457: cache: an optional cache service client
that implements get() and set()

On 2012/07/13 15:24:40, jcgregorio_google wrote:
> An

Done.

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clientsecrets.py
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/11002/tests/test_oauth2client_clientsecrets.py#newcode73
tests/test_oauth2client_clientsecrets.py:73: 'afilethatisntthere.json'))

On 2012/07/13 15:24:40, jcgregorio_google wrote:
> Looks like there are multiple tests that need a filename guaranteed
not to
> exist. Break out as a module level constant.

Done.

http://codereview.appspot.com/6349087/

[jcgre...@google.com]

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appengine.py
File tests/test_oauth2client_appengine.py (right):

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appengine.py#newcode78
tests/test_oauth2client_appengine.py:78:
two blank lines

http://codereview.appspot.com/6349087/

[al...@cloudware.it]

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appengine.py
File tests/test_oauth2client_appengine.py (right):

http://codereview.appspot.com/6349087/diff/5011/tests/test_oauth2client_appengine.py#newcode78
tests/test_oauth2client_appengine.py:78:

On 2012/07/16 16:00:14, jcgregorio_google wrote:
> two blank lines

Done.

http://codereview.appspot.com/6349087/

[jcgre...@google.com]

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clientsecrets.py
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clientsecrets.py#newcode127
tests/test_oauth2client_clientsecrets.py:127:
self.assertIsNone(self.cache_mock.last_set_ns)
assertIsNone only works on Python 2.7, please use assertNotEqual.

http://codereview.appspot.com/6349087/

[al...@cloudware.it]

Sorry, forgot about < 2.7

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clientsecrets.py
File tests/test_oauth2client_clientsecrets.py (right):

http://codereview.appspot.com/6349087/diff/17001/tests/test_oauth2client_clientsecrets.py#newcode127
tests/test_oauth2client_clientsecrets.py:127:
self.assertIsNone(self.cache_mock.last_set_ns)

On 2012/07/16 16:17:15, jcgregorio_google wrote:
> assertIsNone only works on Python 2.7, please use assertNotEqual.

Done.

http://codereview.appspot.com/6349087/

[jcgre...@google.com]

LGTM.

Committed at
http://code.google.com/p/google-api-python-client/source/detail?r=9e3a933696257a1419ecfe7112c6f5fbb094e2ef