Resumable upload

[aiuto, 2011-11-21 15:41:12]

http://codereview.appspot.com/5417051/diff/2001/apiclient/discovery.py
File apiclient/discovery.py (right):

http://codereview.appspot.com/5417051/diff/2001/apiclient/discovery.py#newcod...
apiclient/discovery.py:496: # we will then use to split up the resulting
multipart/related body.
I'm seriously confused by this comment and the code.
It appears that you actually have random *payload*.

That may be right, but the comment is not helping me
understand how that helps.

http://codereview.appspot.com/5417051/diff/2001/apiclient/discovery.py#newcod...
apiclient/discovery.py:513: headers['content-type'] = ('multipart/related; '
Could this be done by http.py? That is, you just
give http the unique part of the content divider
and it builds the string inside.

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py
File apiclient/http.py (right):

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode233
apiclient/http.py:233: is a resumable multipart upload.
Something feels funny about the multipart_boundary here.
It seems you have the caller putting it in the headers, then using it in http
only for appending to the last request packet.

It seems like this might be handled as an element of the MediaUpload object,
rather than an independent field.

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode289
apiclient/http.py:289: body=self.body,
indentation

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode305
apiclient/http.py:305: resumable=True)
indentation

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode346
apiclient/http.py:346: data += self.multipart_boundary
If I see it right, this is the only use of self.multipart_boundary, so it could
just as easily be self.resumable.multipart_boundary

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode358
apiclient/http.py:358: elif resp.status == 308:
Are there constants for 300-308? Those are not too well known to people.

[jcgregorio_google, 2011-11-22 14:27:38]

http://codereview.appspot.com/5417051/diff/2001/apiclient/discovery.py
File apiclient/discovery.py (right):

http://codereview.appspot.com/5417051/diff/2001/apiclient/discovery.py#newcod...
apiclient/discovery.py:496: # we will then use to split up the resulting
multipart/related body.
Rearranged code to make it clearer and expanded the comments.

On 2011/11/21 15:41:12, aiuto wrote:
> I'm seriously confused by this comment and the code.
> It appears that you actually have random *payload*.
> 
> That may be right, but the comment is not helping me
> understand how that helps.

http://codereview.appspot.com/5417051/diff/2001/apiclient/discovery.py#newcod...
apiclient/discovery.py:513: headers['content-type'] = ('multipart/related; '
Fixed as part of the rearrangement done above.

On 2011/11/21 15:41:12, aiuto wrote:
> Could this be done by http.py? That is, you just
> give http the unique part of the content divider
> and it builds the string inside.

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py
File apiclient/http.py (right):

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode233
apiclient/http.py:233: is a resumable multipart upload.
Actually the multipart boundary is just sent as part of the content-type header
so I don't have a need to pass it over separately. Done.

On 2011/11/21 15:41:12, aiuto wrote:
> Something feels funny about the multipart_boundary here.
> It seems you have the caller putting it in the headers, then using it in http
> only for appending to the last request packet.
> 
> It seems like this might be handled as an element of the MediaUpload object,
> rather than an independent field.

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode289
apiclient/http.py:289: body=self.body,
On 2011/11/21 15:41:12, aiuto wrote:
> indentation

Done.

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode305
apiclient/http.py:305: resumable=True)
On 2011/11/21 15:41:12, aiuto wrote:
> indentation

Done.

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode346
apiclient/http.py:346: data += self.multipart_boundary
Done. Now pulled out of the content-type header. 

On 2011/11/21 15:41:12, aiuto wrote:
> If I see it right, this is the only use of self.multipart_boundary, so it
could
> just as easily be self.resumable.multipart_boundary

http://codereview.appspot.com/5417051/diff/2001/apiclient/http.py#newcode358
apiclient/http.py:358: elif resp.status == 308:
Not for 308. Made the comment better.

On 2011/11/21 15:41:12, aiuto wrote:
> Are there constants for 300-308? Those are not too well known to people.

[aiuto, 2011-11-22 14:40:42]

lgtm

[jcgregorio_google, 2011-11-22 15:02:00]

On 2011/11/22 14:40:42, aiuto wrote:
> lgtm

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