Moving logic out of closure in createMethod and into helper methods.

Moving logic out of closure in createMethod and into helper methods.

[dhermes, 2013-02-26 19:57:11]

1) This is the first commit in a series of commits leading to fully ditching the
closure and using a callable object for methods.

2) I'd like to add some tests to this, but haven't been able to get tox to run.

3) Any input on which sorts of things you'd like to see tested? Simple unit
tests for each private helper introduced?

[jcgregorio_google, 2013-02-26 21:23:47]

On Tue, Feb 26, 2013 at 2:57 PM,  <dhermes@google.com> wrote:
> 1) This is the first commit in a series of commits leading to fully
> ditching the closure and using a callable object for methods.
>
> 2) I'd like to add some tests to this, but haven't been able to get tox
> to run.
>
> 3) Any input on which sorts of things you'd like to see tested? Simple
> unit tests for each private helper introduced?

Tests for private helps should be fine.

 -joe

>
> https://codereview.appspot.com/7375057/

[dhermes, 2013-02-27 03:08:19]

Adding docstrings and tests.

[jcgregorio_google, 2013-02-27 16:51:30]

https://codereview.appspot.com/7375057/diff/5001/apiclient/discovery.py
File apiclient/discovery.py (right):

https://codereview.appspot.com/7375057/diff/5001/apiclient/discovery.py#newco...
apiclient/discovery.py:328: return _MEDIA_PATH_TEMPLATE % {
I'm not sure breaking out _MEDIA_PATH_TEMPLATE as a constant is a benefit here,
it's a single line of text and it isn't used anywhere else in the code.

https://codereview.appspot.com/7375057/diff/5001/apiclient/discovery.py#newco...
apiclient/discovery.py:372: def _fix_up_body_parameter(method_desc, http_method,
parameters):
This fn is only 4 lines of code, it really should be folded into
_fix_up_parameters since that's what it's doing.

[dhermes, 2013-02-27 17:10:28]

Getting rid of fix body param and a constant and updating tests.

[dhermes, 2013-02-27 17:12:21]

https://codereview.appspot.com/7375057/diff/5001/apiclient/discovery.py
File apiclient/discovery.py (right):

https://codereview.appspot.com/7375057/diff/5001/apiclient/discovery.py#newco...
apiclient/discovery.py:328: return _MEDIA_PATH_TEMPLATE % {
On 2013/02/27 16:51:30, jcgregorio_google wrote:
> I'm not sure breaking out _MEDIA_PATH_TEMPLATE as a constant is a benefit
here,
> it's a single line of text and it isn't used anywhere else in the code.

Done.

https://codereview.appspot.com/7375057/diff/5001/apiclient/discovery.py#newco...
apiclient/discovery.py:372: def _fix_up_body_parameter(method_desc, http_method,
parameters):
On 2013/02/27 16:51:30, jcgregorio_google wrote:
> This fn is only 4 lines of code, it really should be folded into
> _fix_up_parameters since that's what it's doing.

Done.

[jcgregorio_google, 2013-02-27 17:52:57]

https://codereview.appspot.com/7375057/diff/3002/apiclient/discovery.py
File apiclient/discovery.py (right):

https://codereview.appspot.com/7375057/diff/3002/apiclient/discovery.py#newco...
apiclient/discovery.py:88: # TODO(dhermes): Determine if 'userip' is still
needed
userip is still needed for legacy application. Could be dropped if we ever went
to a v2 of the client library.

https://codereview.appspot.com/7375057/diff/3002/tests/test_discovery.py
File tests/test_discovery.py (right):

https://codereview.appspot.com/7375057/diff/3002/tests/test_discovery.py#newc...
tests/test_discovery.py:216: ten_gb = 10737418240  # 10 * 2**30
ten_gb = 10 * 2**30

here and below

[dhermes, 2013-02-27 18:08:46]

Some more review comments addressed.

[dhermes, 2013-02-27 18:09:32]

https://codereview.appspot.com/7375057/diff/3002/apiclient/discovery.py
File apiclient/discovery.py (right):

https://codereview.appspot.com/7375057/diff/3002/apiclient/discovery.py#newco...
apiclient/discovery.py:88: # TODO(dhermes): Determine if 'userip' is still
needed
Changed the TODO to reflect this.

On 2013/02/27 17:52:57, jcgregorio_google wrote:
> userip is still needed for legacy application. Could be dropped if we ever
went
> to a v2 of the client library.

https://codereview.appspot.com/7375057/diff/3002/tests/test_discovery.py
File tests/test_discovery.py (right):

https://codereview.appspot.com/7375057/diff/3002/tests/test_discovery.py#newc...
tests/test_discovery.py:216: ten_gb = 10737418240  # 10 * 2**30
On 2013/02/27 17:52:57, jcgregorio_google wrote:
> ten_gb = 10 * 2**30
> 
> here and below

Done.

[jcgregorio_google, 2013-02-27 18:14:14]

LGTM

On 2013/02/27 18:09:32, dhermes wrote:
> https://codereview.appspot.com/7375057/diff/3002/apiclient/discovery.py
> File apiclient/discovery.py (right):
> 
>
https://codereview.appspot.com/7375057/diff/3002/apiclient/discovery.py#newco...
> apiclient/discovery.py:88: # TODO(dhermes): Determine if 'userip' is still
> needed
> Changed the TODO to reflect this.
> 
> On 2013/02/27 17:52:57, jcgregorio_google wrote:
> > userip is still needed for legacy application. Could be dropped if we ever
> went
> > to a v2 of the client library.
> 
> https://codereview.appspot.com/7375057/diff/3002/tests/test_discovery.py
> File tests/test_discovery.py (right):
> 
>
https://codereview.appspot.com/7375057/diff/3002/tests/test_discovery.py#newc...
> tests/test_discovery.py:216: ten_gb = 10737418240  # 10 * 2**30
> On 2013/02/27 17:52:57, jcgregorio_google wrote:
> > ten_gb = 10 * 2**30
> > 
> > here and below
> 
> Done.

[dhermes, 2013-02-27 18:19:39]

Added in
https://code.google.com/p/google-api-python-client/source/detail?r=54f04f3c92...