-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an SSLContextAdapter #231
base: master
Are you sure you want to change the base?
Conversation
An adapter that allows more fine-grained control over the encryption parameters used in `requests` by passing an `SSLContext` object directly. Originally borne from the desire to test a different implementation of Python's `ssl` module's `create_default_context()` with `requests`.
On older versions of python, `ssl.PROTOCOL_TLS` is not defined. Also, older requests versions caused the tests to fail.
|
||
|
||
class SSLContextAdapter(HTTPAdapter): | ||
"""An adapter that lets the user inject a custom SSL context for all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the style of other class and module docstrings in this library.
# -*- coding: utf-8 -*- | ||
"""This file contains an implementation of the SSLContextAdapter. | ||
|
||
It requires a version of requests >= 2.4.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at other submodules for the library's style and follow that
self.ssl_context = None | ||
super(SSLContextAdapter, self).__setstate__(state) | ||
|
||
def init_poolmanager(self, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add docstrings to these methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sigmavirus24,
I'm not sure what kind of information would be expected here in a docstring.
The only useful information I could come up with would be something like "Calls superclass' init_poolmanager method with an extra arg on requests >= 2.4.0". This seems redundant, given that the methods themselves are really not complicated.
Adding docstrings here would also be inconsistent with the implementations of the other Adapters in this module.
If all methods are supposed to have some kind of docstring despite these arguments, I'll be happy to provide some - just trying to understand the reasoning here.
Regards
Nikos
@pytest.mark.skipif(requests.__build__ < 0x020400, | ||
reason="Test case for newer requests versions.") | ||
@mock.patch.object(HTTPAdapter, 'init_poolmanager') | ||
def test_ssl_context_arg_is_passed_on_newer_requests(init_poolmanager): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add docstrings to the test functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also my comment on docstrings in the actual module - I've added slightly wordier descriptions as docstrings now. Is that what you expected @sigmavirus24?
@pytest.mark.skipif(requests.__build__ >= 0x020400, | ||
reason="Test case for older requests versions.") | ||
@mock.patch.object(HTTPAdapter, 'init_poolmanager') | ||
def test_ssl_context_arg_is_not_passed_on_older_requests(init_poolmanager): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
Ping @sigmavirus24 could you have another look? Thanks :) |
An adapter that allows more fine-grained control over the encryption
parameters used in
requests
by passing anSSLContext
objectdirectly.
Originally borne from the desire to test a different implementation of
Python's
ssl
module'screate_default_context()
withrequests
.