Batch

[aiuto, 2011-11-28 21:17:46]

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py
File apiclient/http.py (right):

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode155
apiclient/http.py:155: resumable=False):
line wrap? or does PEP-8 require it on the 80th col?

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode421
apiclient/http.py:421: batch_uri = 'https://www.googleapis.com/batch'
Can this be lazy defaulted by looking at the uri in one of the requests?

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode450
apiclient/http.py:450: return '<' + '%s+%s' % (self._base_id, urllib.quote(id_))
+ '>'
If you are going to use '%s+%s' % ... why not make
it '<%s+%s>' % ...

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode541
apiclient/http.py:541: def execute(self, http=None):
maybe order this after add, so that the public methods are separate from the
protected ones.

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode570
apiclient/http.py:570: for request_id in self._requests.keys():
Shouldn't they execute in the order they were added.

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode615
apiclient/http.py:615: # gzipping individual response bodies.
Is that a TODO or a behavior you should account for forever?

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode651
apiclient/http.py:651: the callback with the response.
You might want some discussion about the semantics here. I see that you can put
an ID on a few things and then not on others. That's useful, but if I do
add(..., request_id = 1)
add(...)
add(...)
add(..., request_id = 2)
I am going to get an error, and I won't know why. Maybe you could go negative on
the auto_id ?

OTOH, once I mix auto and explicit, I think it's fair to say I voided the
warranty.

[jcgregorio_google, 2011-11-29 14:16:34]

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py
File apiclient/http.py (right):

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode155
apiclient/http.py:155: resumable=False):
On 2011/11/28 21:17:46, aiuto wrote:
> line wrap? or does PEP-8 require it on the 80th col?

Done.

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode421
apiclient/http.py:421: batch_uri = 'https://www.googleapis.com/batch'
Actually this should go away once the batch information is added to discovery.

On 2011/11/28 21:17:46, aiuto wrote:
> Can this be lazy defaulted by looking at the uri in one of the requests?

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode450
apiclient/http.py:450: return '<' + '%s+%s' % (self._base_id, urllib.quote(id_))
+ '>'
On 2011/11/28 21:17:46, aiuto wrote:
> If you are going to use '%s+%s' % ... why not make
> it '<%s+%s>' % ...

Done.

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode541
apiclient/http.py:541: def execute(self, http=None):
On 2011/11/28 21:17:46, aiuto wrote:
> maybe order this after add, so that the public methods are separate from the
> protected ones.

Done.

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode570
apiclient/http.py:570: for request_id in self._requests.keys():
On 2011/11/28 21:17:46, aiuto wrote:
> Shouldn't they execute in the order they were added.

Done.

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode615
apiclient/http.py:615: # gzipping individual response bodies.
Right now stack has a bug where the individual response bodies are always
gzipped. This should go away once that bug is fixed.

If we want to keep the behavior as a defensive measure then it's not complete.
For example, an XML or protobuf response wouldn't trigger the unzip.

On 2011/11/28 21:17:46, aiuto wrote:
> Is that a TODO or a behavior you should account for forever?

http://codereview.appspot.com/5434059/diff/1/apiclient/http.py#newcode651
apiclient/http.py:651: the callback with the response.
On 2011/11/28 21:17:46, aiuto wrote:
> You might want some discussion about the semantics here. I see that you can
put
> an ID on a few things and then not on others. That's useful, but if I do
> add(..., request_id = 1)
> add(...)
> add(...)
> add(..., request_id = 2)
> I am going to get an error, and I won't know why. Maybe you could go negative
on
> the auto_id ?
> 
> OTOH, once I mix auto and explicit, I think it's fair to say I voided the
> warranty.

Done.

[aiuto, 2011-11-29 14:52:00]

lgtm

[jcgregorio_google, 2011-11-30 16:01:03]

On 2011/11/29 14:52:00, aiuto wrote:
> lgtm

Committed in
http://code.google.com/p/google-api-python-client/source/detail?r=83353327707...