Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(370)

Issue 6506074: Making httplib2.Http instances pickleable. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 7 months ago by dhermes
Modified:
1 year, 4 months ago
CC:
httplib2-dev_googlegroups.com
Visibility:
Public.

Description

Making httplib2.Http instances pickleable.

Patch Set 1 #

Total comments: 2

Patch Set 2 : Adding unit tests for pickled Http objects. #

Total comments: 2

Patch Set 3 : Making sure connections gets dropped and making tests less brittle. #

Total comments: 2

Patch Set 4 : Adding in missing args and kwargs to ResponseDict. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -75 lines) Patch
M python2/httplib2/__init__.py View 1 2 3 7 chunks +83 lines, -68 lines 0 comments Download
M python2/httplib2test.py View 1 2 7 chunks +42 lines, -7 lines 0 comments Download
M python3/httplib2/__init__.py View 1 2 1 chunk +14 lines, -0 lines 2 comments Download
M python3/httplib2test.py View 1 2 2 chunks +37 lines, -0 lines 0 comments Download

Messages

Total messages: 18
dhermes
1 year, 7 months ago #1
jcgregorio_google
Needs unit tests, including one where you attach an attribute to the request method. https://codereview.appspot.com/6506074/diff/1/python2/httplib2/__init__.py ...
1 year, 7 months ago #2
dhermes
https://codereview.appspot.com/6506074/diff/1/python2/httplib2/__init__.py File python2/httplib2/__init__.py (right): https://codereview.appspot.com/6506074/diff/1/python2/httplib2/__init__.py#newcode1242 python2/httplib2/__init__.py:1242: # credentials which handle auth On 2012/09/05 15:20:07, jcgregorio_google ...
1 year, 7 months ago #3
dhermes
Any issue here? On Wed, Sep 5, 2012 at 8:23 AM, <dhermes@google.com> wrote: > > ...
1 year, 7 months ago #4
jcgregorio_google
Yes, it still needs unit tests like I mentioned in my first email. -joe On ...
1 year, 7 months ago #5
dhermes
Sorry it took me 6 days. I have added unit tests, though I'm not sure ...
1 year, 7 months ago #6
jcgregorio_google
https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py File python2/httplib2test.py (right): https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py#newcode44 python2/httplib2test.py:44: pickleTemplate = ( This is going to be fragile. ...
1 year, 7 months ago #7
dhermes
https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py File python2/httplib2test.py (right): https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py#newcode44 python2/httplib2test.py:44: pickleTemplate = ( I got rid of the fragility ...
1 year, 7 months ago #8
jcgregorio_google
https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py File python2/httplib2/__init__.py (right): https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py#newcode1075 python2/httplib2/__init__.py:1075: return super(ResponseDict, self).__init__(*args, **kwargs) Where do *args, and **kwargs ...
1 year, 7 months ago #9
dhermes
https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py File python2/httplib2/__init__.py (right): https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py#newcode1075 python2/httplib2/__init__.py:1075: return super(ResponseDict, self).__init__(*args, **kwargs) Good catch. I must have ...
1 year, 7 months ago #10
dhermes
Joe, Have you had a chance to see the latest diffs? On Thu, Sep 13, ...
1 year, 7 months ago #11
jcgregorio_google
https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py File python3/httplib2/__init__.py (right): https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py#newcode801 python3/httplib2/__init__.py:801: def __init__(self, cache=None, timeout=None, proxy_info=None, Looks like defaulting to ...
1 year, 7 months ago #12
dhermes
https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py File python3/httplib2/__init__.py (right): https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py#newcode801 python3/httplib2/__init__.py:801: def __init__(self, cache=None, timeout=None, proxy_info=None, Joe, This never existed ...
1 year, 7 months ago #13
jcgregorio_google
LGTM Conditional on getting that next CL to fix up python3 :) On 2012/09/17 17:48:19, ...
1 year, 7 months ago #14
jcgregorio_google
Are you sure you are sync'd to head? I get failed hunks when I try ...
1 year, 7 months ago #15
dhermes
I did sync to head. I am also getting failed hunks when I try to ...
1 year, 7 months ago #16
dhermes
Does this work? On Tue, Sep 18, 2012 at 8:30 AM, Daniel Hermes <dhermes@google.com> wrote: ...
1 year, 7 months ago #17
jcgregorio_google
1 year, 6 months ago #18
Committed at
http://code.google.com/p/httplib2/source/detail?r=1f1f7d61957691b482eea7b5c19...

On 2012/09/20 21:44:16, dhermes wrote:
> Does this work?
> 
> 
> On Tue, Sep 18, 2012 at 8:30 AM, Daniel Hermes <mailto:dhermes@google.com>
wrote:
> 
> > I did sync to head. I am also getting failed hunks when I try to apply to
> > a clean repo, but it is unclear why they are failing.
> >
> > The first hunk is an "import pickle" statement that falls in alphabetical
> > order in python3/httplib2test.py and the second is just the tests I added.
> >
> > I don't know enough about mercurial/rietveld to understand why this is
> > failing. I manually applied the failed hunks and ran an "hg diff >
> > manual_apply.diff" and have attached that diff, which will successfully
> > apply.
> >
> > Let me know if that works for you.
> >
> >
> > On Tue, Sep 18, 2012 at 7:04 AM, <mailto:jcgregorio@google.com> wrote:
> >
> >> Are you sure you are sync'd to head? I get failed hunks
> >> when I try to apply this.
> >>
> >>
> >> On 2012/09/18 14:00:33, jcgregorio_google wrote:
> >>
> >>> LGTM
> >>>
> >>
> >>  Conditional on getting that next CL to fix up python3 :)
> >>>
> >>
> >>  On 2012/09/17 17:48:19, dhermes wrote:
> >>> >
> >>>
> >> https://codereview.appspot.**com/6506074/diff/14002/**
> >>
>
python3/httplib2/__init__.py<https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py>
> >>
> >>> > File python3/httplib2/__init__.py (right):
> >>> >
> >>> >
> >>>
> >>
> >> https://codereview.appspot.**com/6506074/diff/14002/**
> >>
>
python3/httplib2/__init__.py#**newcode801<https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py#newcode801>
> >>
> >>> > python3/httplib2/__init__.py:**801: def __init__(self, cache=None,
> >>>
> >> timeout=None,
> >>
> >>> > proxy_info=None,
> >>> > Joe,
> >>> >
> >>> > This never existed before. Can we add this in a subsequent change?
> >>>
> >> The point
> >>
> >>> of
> >>> > this change was to make Http instances pickle-able.
> >>> >
> >>> > I'll be happy to start working on the next change right away.
> >>> >
> >>> > On 2012/09/17 17:46:22, jcgregorio_google wrote:
> >>> > > Looks like defaulting to proxyinfo from environment never made it
> >>>
> >> into the
> >>
> >>> > code,
> >>> > > even though that's what the doc comments say. Can you update the
> >>>
> >> proxy
> >>
> >>> > handling
> >>> > > for Python 3 be the same as Python 2?
> >>>
> >>
> >>
> >>
> >>
>
https://codereview.appspot.**com/6506074/%3Chttps://codereview.appspot.com/65...>
> >>
> >
> >
> >
> > --
> > Daniel Hermes
> > Developer Programs Engineer
> >
> 
> 
> 
> -- 
> Daniel Hermes
> Developer Programs Engineer
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5