Flows no longer need to be saved between uses.

Begin moving the redirect_uri out of step1() and into the constructor, but in a way that allows backwards compatibility. 

In the process also add in oauth2client.util.positional that helps catch all the places where I've been sloppy with optional params.

[alexv, 2012-08-01 21:06:05]

Not really a code review. Just wanted to say great changes, Joe.

http://codereview.appspot.com/6441056/diff/6001/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6441056/diff/6001/oauth2client/appengine.py#new...
oauth2client/appengine.py:480: @login_required
It's a pity this works with Google Accounts only, plus makes it kind of a
"double" auth: first login, then authorize. There might be a more generic
solution where a developer provides its own method for "get_current_user" and/or
@login_required. I'll try to think of something for a next release maybe, if it
somehow fits into oauth2client roadmap.

[jcgregorio_google, 2012-08-01 21:08:38]

http://codereview.appspot.com/6441056/diff/6001/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6441056/diff/6001/oauth2client/appengine.py#new...
oauth2client/appengine.py:480: @login_required
Yeah, there's an open issue to create a similar decorator for Django apps:

  http://code.google.com/p/google-api-python-client/issues/detail?id=99

Which would cover many more platforms, but doesn't make it a completely generic
mechanism.


On 2012/08/01 21:06:05, crhyme wrote:
> It's a pity this works with Google Accounts only, plus makes it kind of a
> "double" auth: first login, then authorize. There might be a more generic
> solution where a developer provides its own method for "get_current_user"
and/or
> @login_required. I'll try to think of something for a next release maybe, if
it
> somehow fits into oauth2client roadmap.

[Ali Afshar, 2012-08-02 02:20:07]

http://codereview.appspot.com/6441056/diff/6001/apiclient/discovery.py
File apiclient/discovery.py (right):

http://codereview.appspot.com/6441056/diff/6001/apiclient/discovery.py#newcod...
apiclient/discovery.py:143: @util.positional(2)
I think this is a backwards-incompatible change. How do we know people are not
calling these with all positional arguments? I might remove the decorator but
leave the changes to call all internal functions with named arguments.

A better option might be to change the decorator to emit deprecation warnings
when the wrong number of positional arguments are passed. Add to that a flag to
enable strict mode  that would error out instead of warning.

http://codereview.appspot.com/6441056/diff/6001/samples/appengine/client_secr...
File samples/appengine/client_secrets.json (right):

http://codereview.appspot.com/6441056/diff/6001/samples/appengine/client_secr...
samples/appengine/client_secrets.json:3: "client_id":
"887851474342-b91a0iokgfe05t24npinqopu6rkhru5n.apps.googleusercontent.com",
Are these OK to be here?

[jcgregorio_google, 2012-08-02 17:21:05]

Good catch, positional errors now just log a warning by default.

http://codereview.appspot.com/6441056/diff/6001/apiclient/discovery.py
File apiclient/discovery.py (right):

http://codereview.appspot.com/6441056/diff/6001/apiclient/discovery.py#newcod...
apiclient/discovery.py:143: @util.positional(2)
Fixed. Added a flag to control how positional args are flagged, either raising
an exception, logging a warning, or ignoring completely. Also changed it so that
unit tests are always run in strict mode.

On 2012/08/02 02:20:07, Ali Afshar wrote:
> I think this is a backwards-incompatible change. How do we know people are not
> calling these with all positional arguments? I might remove the decorator but
> leave the changes to call all internal functions with named arguments.
> 
> A better option might be to change the decorator to emit deprecation warnings
> when the wrong number of positional arguments are passed. Add to that a flag
to
> enable strict mode  that would error out instead of warning.

http://codereview.appspot.com/6441056/diff/6001/samples/appengine/client_secr...
File samples/appengine/client_secrets.json (right):

http://codereview.appspot.com/6441056/diff/6001/samples/appengine/client_secr...
samples/appengine/client_secrets.json:3: "client_id":
"887851474342-b91a0iokgfe05t24npinqopu6rkhru5n.apps.googleusercontent.com",
On 2012/08/02 02:20:07, Ali Afshar wrote:
> Are these OK to be here?

Done.

[rmistry, 2012-08-03 13:06:35]

http://codereview.appspot.com/6441056/diff/8002/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6441056/diff/8002/oauth2client/appengine.py#new...
oauth2client/appengine.py:376: def _create_flow(self, request_handler):
Missing method level documentation

http://codereview.appspot.com/6441056/diff/8002/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/6441056/diff/8002/oauth2client/client.py#newcod...
oauth2client/client.py:1051: def step1_get_authorize_url(self,
redirect_uri=None):
Missing @util.position(1)

http://codereview.appspot.com/6441056/diff/8002/oauth2client/client.py#newcod...
oauth2client/client.py:1159: @util.positional(3)
Shouldn't this be @util.position(2)?

http://codereview.appspot.com/6441056/diff/8002/runtests.py
File runtests.py (right):

http://codereview.appspot.com/6441056/diff/8002/runtests.py#newcode9
runtests.py:9: import oauth2client.util
util is not used in this module?

[jcgregorio_google, 2012-08-03 13:30:43]

http://codereview.appspot.com/6441056/diff/8002/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6441056/diff/8002/oauth2client/appengine.py#new...
oauth2client/appengine.py:376: def _create_flow(self, request_handler):
On 2012/08/03 13:06:35, rmistry wrote:
> Missing method level documentation

Done.

http://codereview.appspot.com/6441056/diff/8002/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/6441056/diff/8002/oauth2client/client.py#newcod...
oauth2client/client.py:1051: def step1_get_authorize_url(self,
redirect_uri=None):
On 2012/08/03 13:06:35, rmistry wrote:
> Missing @util.position(1)

Done.

http://codereview.appspot.com/6441056/diff/8002/oauth2client/client.py#newcod...
oauth2client/client.py:1159: @util.positional(3)
On 2012/08/03 13:06:35, rmistry wrote:
> Shouldn't this be @util.position(2)?

Done.

http://codereview.appspot.com/6441056/diff/8002/runtests.py
File runtests.py (right):

http://codereview.appspot.com/6441056/diff/8002/runtests.py#newcode9
runtests.py:9: import oauth2client.util
It needs to be imported here to get its gflags visible. Added a comment.


On 2012/08/03 13:06:35, rmistry wrote:
> util is not used in this module?

[aiuto, 2012-08-03 18:42:46]

just some little nits

http://codereview.appspot.com/6441056/diff/11003/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6441056/diff/11003/oauth2client/appengine.py#ne...
oauth2client/appengine.py:578: @util.positional(3)
positional(2) ?

http://codereview.appspot.com/6441056/diff/11003/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/6441056/diff/11003/oauth2client/client.py#newco...
oauth2client/client.py:487: id_token=data.get('id_token', None))
Unused assignment?

http://codereview.appspot.com/6441056/diff/11003/oauth2client/client.py#newco...
oauth2client/client.py:933: http=None, user_agent=None,
indentation is off

http://codereview.appspot.com/6441056/diff/11003/tests/test_oauth2client.py
File tests/test_oauth2client.py (left):

http://codereview.appspot.com/6441056/diff/11003/tests/test_oauth2client.py#o...
tests/test_oauth2client.py:244: authorize_url =
self.flow.step1_get_authorize_url('OOB_CALLBACK_URN')
Is this string defined some where that you could use?

[jcgregorio_google, 2012-08-03 19:28:28]

http://codereview.appspot.com/6441056/diff/11003/oauth2client/appengine.py
File oauth2client/appengine.py (right):

http://codereview.appspot.com/6441056/diff/11003/oauth2client/appengine.py#ne...
oauth2client/appengine.py:578: @util.positional(3)
On 2012/08/03 18:42:46, aiuto wrote:
> positional(2) ?

Done.

http://codereview.appspot.com/6441056/diff/11003/oauth2client/client.py
File oauth2client/client.py (right):

http://codereview.appspot.com/6441056/diff/11003/oauth2client/client.py#newco...
oauth2client/client.py:487: id_token=data.get('id_token', None))
No, that's an optional parameter to the OAuth2Credentials constructor.

On 2012/08/03 18:42:46, aiuto wrote:
> Unused assignment?

http://codereview.appspot.com/6441056/diff/11003/oauth2client/client.py#newco...
oauth2client/client.py:933: http=None, user_agent=None,
On 2012/08/03 18:42:46, aiuto wrote:
> indentation is off

Done.

http://codereview.appspot.com/6441056/diff/11003/tests/test_oauth2client.py
File tests/test_oauth2client.py (left):

http://codereview.appspot.com/6441056/diff/11003/tests/test_oauth2client.py#o...
tests/test_oauth2client.py:244: authorize_url =
self.flow.step1_get_authorize_url('OOB_CALLBACK_URN')
Yes, there is, and I even imported it, no idea why I then put all of these in
quotes :/

On 2012/08/03 18:42:46, aiuto wrote:
> Is this string defined some where that you could use?

[aiuto, 2012-08-03 19:39:29]

lgtm

[Ali Afshar, 2012-08-07 21:59:22]

On 2012/08/03 19:39:29, aiuto wrote:
> lgtm

LGTM