Skip to content
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 cchardet support and use chardet as fallback #4115

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 34 additions & 18 deletions requests/__init__.py
Expand Up @@ -39,15 +39,18 @@
:copyright: (c) 2017 by Kenneth Reitz.
:license: Apache 2.0, see LICENSE for more details.
"""
def get_version(package):
version = package.__version__.split('.')[:3]
if len(version) == 2:
version.append('0')
major, minor, patch = version
major, minor, patch = int(major), int(minor), int(patch)
return major, minor, patch

# Check urllib3 for compatibility.
import urllib3
urllib3_version = urllib3.__version__.split('.')
# Sometimes, urllib3 only reports its version as 16.1.
if len(urllib3_version) == 2:
urllib3_version.append('0')
major, minor, patch = urllib3_version
major, minor, patch = int(major), int(minor), int(patch)

major, minor, patch = get_version(urllib3)
# urllib3 >= 1.21.1, < 1.22
try:
assert major == 1
Expand All @@ -56,18 +59,33 @@
except AssertionError:
raise RuntimeError('Requests dependency \'urllib3\' must be version >= 1.21.1, < 1.22!')

import warnings

# Check chardet for compatibility.
import chardet
major, minor, patch = chardet.__version__.split('.')[:3]
major, minor, patch = int(major), int(minor), int(patch)
# chardet >= 3.0.2, < 3.1.0
try:
assert major == 3
assert minor < 1
assert patch >= 2
except AssertionError:
raise RuntimeError('Requests dependency \'chardet\' must be version >= 3.0.2, < 3.1.0!')
# Check cchardet for compatibility.
import cchardet as chardet
major, minor, patch = get_version(chardet)
# cchardet >= 2.1.0
try:
assert major == 2
assert minor >= 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not:

assert 1 <= minor < 2

assert minor < 2
assert patch >= 0
except AssertionError:
warnings.warn('Requests dependency \'cchardet\' must be version >= 2.1.0, < 2.2.0! Falling back to chardet')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a dependency of Requests, it's an optional speed-up. Please revise

raise

except (ImportError, AssertionError):
# Check chardet for compatibility.
import chardet
major, minor, patch = get_version(chardet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chardet shouldn't be imported in this except block. This will cause extremely confusing tracebacks for users if they can't import either.

# chardet >= 3.0.2, < 3.1.0
try:
assert major == 3
assert minor < 1
assert patch >= 2
except AssertionError:
raise RuntimeError('Requests dependency \'chardet\' must be version >= 3.0.2, < 3.1.0!')

# Attempt to enable urllib3's SNI support, if possible
try:
Expand All @@ -76,8 +94,6 @@
except ImportError:
pass

import warnings

# urllib3's DependencyWarnings should be silenced.
from urllib3.exceptions import DependencyWarning
warnings.simplefilter('ignore', DependencyWarning)
Expand Down
5 changes: 4 additions & 1 deletion requests/compat.py
Expand Up @@ -8,7 +8,10 @@
Python 3.
"""

import chardet
try:
import cchardet as chardet
except ImportError:
import chardet

import sys

Expand Down
12 changes: 10 additions & 2 deletions requests/packages.py
Expand Up @@ -3,8 +3,16 @@
# This code exists for backwards compatibility reasons.
# I don't like it either. Just look the other way. :)

for package in ('urllib3', 'idna', 'chardet'):
locals()[package] = __import__(package)
for package in ('urllib3', 'idna', ('chardet', 'cchardet')):
if isinstance(package, tuple):
package, alt_package = package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to be extremely careful about touching this code, at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This looks safe, but it'd be so much better if this were testable.

try:
locals()[package] = __import__(alt_package)
except ImportError:
locals()[package] = __import__(package)
else:
locals()[package] = __import__(package)

# This traversal is apparently necessary such that the identities are
# preserved (requests.packages.urllib3.* is urllib3.*)
for mod in list(sys.modules):
Expand Down
1 change: 1 addition & 0 deletions setup.py
Expand Up @@ -93,6 +93,7 @@ def run_tests(self):
cmdclass={'test': PyTest},
tests_require=test_requirements,
extras_require={
'cchardet:(python_version == "2.7" or python_version >= "3.4")': ['cchardet>=2.1.0'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're enforcing (in __init__.py) that cchardet be between 2.1.0 and 2.2.0 then that should be written here.

'security': ['pyOpenSSL>=0.14', 'cryptography>=1.3.4', 'idna>=2.0.0'],
'socks': ['PySocks>=1.5.6, !=1.5.7'],
'socks:sys_platform == "win32" and (python_version == "2.7" or python_version == "2.6")': ['win_inet_pton'],
Expand Down