Patch for /oauth2client/client.py

[GoogleCodeExporter]

The return value in OAuth2Credentials.from_json should be an instance of cls 
not of OAuth2Credentials. This makes it a pain to subclass OAuth2Credentials.

Original issue reported on code.google.com by daniel.j.hermes@gmail.com on 13 Dec 2012 at 7:16

Attachments:

• client.py.patch

[GoogleCodeExporter]

Original comment by dhermes@google.com on 13 Dec 2012 at 7:16

[GoogleCodeExporter]

Actually that code should never get called, when you create a subclass of 
OAuth2Credentials you provide a class method 'from_json' that does the right 
thing with the JSON. 

See for example:

http://code.google.com/p/google-api-python-client/source/browse/oauth2client/cli
ent.py#708

Original comment by jcgregorio@google.com on 13 Dec 2012 at 7:27

• Changed state: Waiting

[GoogleCodeExporter]

It's not necessary to duplicate the logic in your provided from_json method. If 
you wanted to use the behavior of from_json and then extend it you could define 
the following:

class MyCredentials(OAuth2Credentials):
  @util.positional(8)
  def __init__(self, ..., one_extra_arg=None):
    super(MyCredentials, self).__init__(...)
    self.one_extra_arg = one_extra_arg

  @classmethod
  def from_json(cls, s):
    my_credentials = super(MyCredentials, cls).from_json(s)
    data = simplejson.loads(s)
    my_credentials.one_extra_arg = data['one_extra_arg']
    return my_credentials

The call to  super(MyCredentials, cls).from_json(s) results in the same parsing 
code, but gives us an instance of MyCredentials, which is what we want.

Original comment by dhermes@google.com on 13 Dec 2012 at 7:37

[GoogleCodeExporter]

"""
The call to  super(MyCredentials, cls).from_json(s) results in the same parsing 
code, but gives us an instance of MyCredentials, which is what we want.
"""

No, I believe it returns an OAuth2Credentials.

Original comment by jcgregorio@google.com on 13 Dec 2012 at 7:52

[GoogleCodeExporter]

Sorry I wasn't clear. With this change, that is what would happen.

But the current way, it returns an instance of the wrong thing and would force 
us to duplicate all the code in OAuth2Credentials.from_json.

Original comment by dhermes@google.com on 13 Dec 2012 at 8:01

[GoogleCodeExporter]

Have you actually tried it? I believe your code will return an 
OAuth2Credentials and not a MyCredentials.

super(MyCredentials, cls) = OAuth2Credentials

Original comment by jcgregorio@google.com on 13 Dec 2012 at 8:03

[GoogleCodeExporter]

Yes I have actually tried it, with the attached patch in place, where in line 
506 I use cls instead of OAuth2Credentials.

from oauth2client.anyjson import simplejson
from oauth2client import util
from oauth2client.client import OAuth2Credentials

class MyCredentials(OAuth2Credentials):

  @util.positional(8)
  def __init__(self, *args, **kwargs):
    self.one_extra_arg = kwargs.pop('one_extra_arg', None)
    super(MyCredentials, self).__init__(*args, **kwargs)

  @classmethod
  def from_json(cls, s):
    my_credentials = super(MyCredentials, cls).from_json(s)
    data = simplejson.loads(s)
    my_credentials.one_extra_arg = data['one_extra_arg']
    return my_credentials

m = MyCredentials('a', 'b', None, None, None, None, None, None,
                  one_extra_arg='M')
J = m.to_json()
mm = MyCredentials.from_json(J)
assert(mm.access_token == m.access_token)
assert(mm.client_id == m.client_id)
assert(mm.client_secret == m.client_secret)
assert(mm.refresh_token == m.refresh_token)
assert(mm.token_expiry == m.token_expiry)
assert(mm.token_uri == m.token_uri)
assert(mm.user_agent == m.user_agent)
assert(mm.id_token == m.id_token)
assert(mm.one_extra_arg == m.one_extra_arg)
assert(isinstance(mm, MyCredentials))

Original comment by dhermes@google.com on 13 Dec 2012 at 8:14

[GoogleCodeExporter]

Oh, OK, now I get it.

Ick.

Original comment by jcgregorio@google.com on 13 Dec 2012 at 8:49

[GoogleCodeExporter]

Ick to the current behavior or to the suggested change?

This change allows people to extend the behavior of from_json without forcing 
them to duplicate 100% of the code contained there.

Original comment by dhermes@google.com on 13 Dec 2012 at 8:51

[GoogleCodeExporter]

So can we make this change?

Original comment by dhermes@google.com on 19 Dec 2012 at 4:13

[GoogleCodeExporter]

Joe,

I am making this "Accepted" and going to send a review.

Any issues?

Original comment by dhermes@google.com on 6 Feb 2013 at 5:23

• Changed state: Accepted

[GoogleCodeExporter]

https://codereview.appspot.com/7389046/

Original comment by dhermes@google.com on 23 Feb 2013 at 2:52

• Changed state: Started

[GoogleCodeExporter]

This issue was closed by revision eeb7e477b352.

Original comment by jcgregorio@google.com on 25 Feb 2013 at 4:20

• Changed state: Fixed