Proposed fix to #1180 (and maybe #932 & #1104) #1902

Merged
merged 3 commits into from Jul 3, 2014

Conversation

Projects
None yet
2 participants
@tnotstar
Contributor

tnotstar commented Jun 27, 2014

This is a rebased final version of a proposed solution to fix
issues #932, #1104 & #1180. Following changes have been done:

  • Implemented a new class PipXmlrpcTransport using a
    contained PipSession object.
  • Modified the pip/commands/search.py to make use of the
    PipXmlrpcTransport class.
  • Properly initialized options for testing SearchCommand:
    • Changed options_mock to an options object built from
      parse_args, to properly initialize default options.
Proposed fix to #1180 (and maybe #932 & #1104)
This is a rebased final version of a proposed solution to fix
issues #932, #1104 & #1180. Following changes have been done:

* Implemented a new class `PipXmlrpcTransport` using a
  contained `PipSession` object.

* Modified the `pip/commands/search.py` to make use of the
  `PipXmlrpcTransport` class.

* Properly initialized options for testing `SearchCommand`:
  - Changed `options_mock` to an `options` object built from
    `parse_args`, to properly initialize default options.
@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jul 3, 2014

Member

Hey, sorry this took so long to review, I've been debugging a really wonky issue.

This looks pretty good, only thing I'd ask is can you change it so that you have the session in a context manager? like:

with self._build_session(...) as session:
    pass  # Do stuff with the session here

This way the connection pool will get closed explicitly? You can see I've made this change to the other commands in #1882.

Member

dstufft commented Jul 3, 2014

Hey, sorry this took so long to review, I've been debugging a really wonky issue.

This looks pretty good, only thing I'd ask is can you change it so that you have the session in a context manager? like:

with self._build_session(...) as session:
    pass  # Do stuff with the session here

This way the connection pool will get closed explicitly? You can see I've made this change to the other commands in #1882.

Close requests session in `search` command
* Added a `with` block to handle `session` life-cycle. As commented
  in #1902
@tnotstar

This comment has been minimized.

Show comment
Hide comment
@tnotstar

tnotstar Jul 3, 2014

Contributor

Perfect! I just committed a little patch for it. Now, we can wait for the Travis' report.

Contributor

tnotstar commented Jul 3, 2014

Perfect! I just committed a little patch for it. Now, we can wait for the Travis' report.

@dstufft

View changes

pip/download.py
+ xmlrpc_client.Transport.__init__(self, use_datetime)
+ index_parts = urlparse.urlparse(index_url)
+ self._scheme = index_parts.scheme
+ session.headers.update({'Content-Type': 'text/xml'})

This comment has been minimized.

@dstufft

dstufft Jul 3, 2014

Member

Hmm, I just noticed this. I'd rather not modify the session in this. Can we just pass this header on each self._session.post call? That way sessions don't start acting differently just because you used them in PipXmlrpcTransport``.

@dstufft

dstufft Jul 3, 2014

Member

Hmm, I just noticed this. I'd rather not modify the session in this. Can we just pass this header on each self._session.post call? That way sessions don't start acting differently just because you used them in PipXmlrpcTransport``.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jul 3, 2014

Member

Real sorry, one more thing I noticed. Also can you add this to changelog? That you've fixed those issues and pip search respects proxies now?

Member

dstufft commented Jul 3, 2014

Real sorry, one more thing I noticed. Also can you add this to changelog? That you've fixed those issues and pip search respects proxies now?

Passing headers per-request in PipXmlrpcTransport
* Changed `PipXmlrpcTransport` to pass headers at request scope
* Added summary line to the changelog
@tnotstar

This comment has been minimized.

Show comment
Hide comment
@tnotstar

tnotstar Jul 3, 2014

Contributor

It's no problem! I think 0dedf2b made the job.

Contributor

tnotstar commented Jul 3, 2014

It's no problem! I think 0dedf2b made the job.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jul 3, 2014

Member

Ok this looks pretty good now. Thanks! The tests are failing but they look transient. I kicked them and we'll see if they pass.

Member

dstufft commented Jul 3, 2014

Ok this looks pretty good now. Thanks! The tests are failing but they look transient. I kicked them and we'll see if they pass.

dstufft added a commit that referenced this pull request Jul 3, 2014

@dstufft dstufft merged commit d359d1a into pypa:develop Jul 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jul 3, 2014

Member

Yay, thanks :)

Member

dstufft commented Jul 3, 2014

Yay, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment