Drop support for EOL Python 2.7 and 3.5

[hugovk]

Fixes #883 (or at least does a lot for it).

This drops support for EOL Python 2.7, upgrades Python syntax for 3.5+ using pyupgrade, and removes a lot of old compatibility code including most six.

There's still some six I was't sure how to replace, like ensure_str and reraise. Maybe they should go in a utility file? And there's still a few other places that can be cleaned up, but this PR is already quite big so they could be done in followups.

One thing: I switched the Travis NOX_SESSION=app_engine job from 2.7 to 3.7, but it's now failing because pytest isn't finding any tests. Any ideas what's needed here?

[codecov]

Codecov Report

Merging #2044 (ebedf9a) into master (deb21b6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2044   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        24    -1     
  Lines         2296      2243   -53     
=========================================
- Hits          2296      2243   -53     

Impacted Files	Coverage Δ
src/urllib3/__init__.py	100.00% <ø> (ø)
src/urllib3/util/__init__.py	100.00% <ø> (ø)
src/urllib3/util/queue.py	100.00% <ø> (ø)
src/urllib3/util/ssltransport.py	100.00% <ø> (ø)
src/urllib3/_collections.py	100.00% <100.00%> (ø)
src/urllib3/connection.py	100.00% <100.00%> (ø)
src/urllib3/connectionpool.py	100.00% <100.00%> (ø)
src/urllib3/contrib/socks.py	100.00% <100.00%> (ø)
src/urllib3/exceptions.py	100.00% <100.00%> (ø)
src/urllib3/fields.py	100.00% <100.00%> (ø)
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update deb21b6...ebedf9a. Read the comment docs.

[sethmlarson]

Nice! Initial comment: can you upgrade the syntax to 3.6+ and drop Python 3.5? I know there's a PR already out for it but I'm thinking specifically about the .format() calls, would prefer going straight to f-strings to avoid git blame noise.

[sethmlarson]

Also can you run black and isort on each commit (rather than having a separate commit for it) so we can eventually rebase merge?

[hugovk]

Python 3.5 dropped as well. Before merge, it might be worth squashing all these into a single commit (or using the squash and merge button in GitHub) to minimise the Git blame noise.

[pquentin]

This is blocked by #2035 so that we can continue testing PyPy support for Python 3.x. We also need to understand and fix the appengine failure.

Thanks for keeping the commits separate for now, reviewing it all the changes together would have been impossible otherwise!

[sethmlarson]

Gonna cc @crwilcox for the App Engine failure in case he has any insights :)

[crwilcox]

At least locally I am seeing the same GAE failures on urllib3:master as this branch. Looking into it now.

[crwilcox]

@sethmlarson I don't believe these tests are relevant for Python 3+ versions. This is "App Engine Standard Py2.7" stuff. I think they should be removed with Py 2.7 support :)

The reason you aren't seeing tests is this chunk in conftest.py

def pytest_ignore_collect(path, config):
    """Skip App Engine tests in python 3 or if no SDK is available."""
    if "appengine" in str(path):
        if not six.PY2:
            return True
        if not os.environ.get("GAE_SDK_PATH"):
            return True
    return False

[hugovk]

@crwilcox Thanks for checking! Can we remove the test/appengine/ directory?

[crwilcox]

@hugovk Please do 😊

[hugovk]

Done and rebased to resolve conflict! Also simplified the nox install.

The required 2.7 and 3.5 "Expected — Waiting for status to be reported" will need to be removed from the branch protection rules.

[sethmlarson]

Some tiny comments, beyond that I've looked at everything and it looks fantastic. Thank you for all this!

[sethmlarson]

LGTM, did you want to review as well @pquentin?

[pquentin]

It looks like coverage is dropping, I wouldn't be comfortable merging this unless we stay at 100%

[pquentin]

I also don't understand why we're removing the AppEngine tests but are keeping the AppEngine code?

@crwilcox Do you know if should:

• remove src/urllib3/contrib/appengine.py?
• remove the code marked with # Platform-specific: Appengine. too?
• remove is_using_tunnel()?

If not, should they be tested differently? (I know nothing about Google App Engine, sorry.)

[hugovk]

It looks like coverage is dropping, I wouldn't be comfortable merging this unless we stay at 100%

_appengine_environ.py

Coverage drops are in _appengine_environ.py:

• https://codecov.io/gh/urllib3/urllib3/src/1510dfadbc7a1d419d22a3a57919387ac13b1b53/src/urllib3/contrib/_appengine_environ.py

If src/urllib3/contrib/appengine.py is deleted, looks like _appengine_environ.py can also be deleted, or at least is_prod_appengine_mvms().

url.py

And also:

• https://codecov.io/gh/urllib3/urllib3/src/1510dfadbc7a1d419d22a3a57919387ac13b1b53/src/urllib3/util/url.py

Is it safe to remove lines 272-273 or do we need a new test?

[pquentin]

I think we want to continue supporting host as bytes, yes, even if this should be a really rare case and would probably never have been supported if we had started with Python 3. +1 for a test

[crwilcox]

@pquentin the appengine code should be specifically for App Engine Standard, Python 2.7. I can review the specific removals to make sure some of this isn't for other App Engine specific tweaks.

[crwilcox]

@pquentin it appears to me htis is all for Python 2 specific AppEngine as APPENGINE_RUNTIME env var is "" on Python 3, and SERVER_SOFTWARE gunicorn.

The conditional logic of /src/urllib3/contrib/_appengine_environ.py makes it seems this code is only used in the Python 2 case.

[hugovk]

url.py

Coverage: I don't think it's possible to run _normalize_host(host, scheme) with a bytes host.

It's called in:

1. util.url.parse_url
2. connectionpool._normalize_host, called by ConnectionPool.__init__
3. connectionpool._normalize_host, called by HTTPConnectionPool.is_same_host

hugovk@04fd14e is an attempt (in another branch) to test those with a bytes host, but they all fail with TypeError:

__ TestConnectionPool.test_same_host[http://google.com/-http://google.com/1] ___
Traceback (most recent call last):
  File "/Users/runner/work/urllib3/urllib3/test/test_connectionpool.py", line 86, in test_same_host
    with connection_from_url(a) as c:
  File "/Users/runner/work/urllib3/urllib3/.nox/test-3-6/lib/python3.6/site-packages/urllib3/connectionpool.py", line 1040, in connection_from_url
    scheme, host, port = get_host(url)
  File "/Users/runner/work/urllib3/urllib3/.nox/test-3-6/lib/python3.6/site-packages/urllib3/util/url.py", line 419, in get_host
    p = parse_url(url)
  File "/Users/runner/work/urllib3/urllib3/.nox/test-3-6/lib/python3.6/site-packages/urllib3/util/url.py", line 354, in parse_url
    if not SCHEME_RE.search(url):
TypeError: cannot use a string pattern on a bytes-like object
_____________________ TestConnectionPool.test_bytes_host1 ______________________
Traceback (most recent call last):
  File "/Users/runner/work/urllib3/urllib3/test/test_connectionpool.py", line 91, in test_bytes_host1
    HTTPConnection(host)
  File "/Users/runner/work/urllib3/urllib3/.nox/test-3-6/lib/python3.6/site-packages/urllib3/connection.py", line 108, in __init__
    _HTTPConnection.__init__(self, *args, **kw)
  File "/Users/runner/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/http/client.py", line 863, in __init__
    (self.host, self.port) = self._get_hostport(host, port)
  File "/Users/runner/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/http/client.py", line 897, in _get_hostport
    i = host.rfind(':')
TypeError: a bytes-like object is required, not 'str'
_____________________ TestConnectionPool.test_bytes_host2 ______________________
Traceback (most recent call last):
  File "/Users/runner/work/urllib3/urllib3/test/test_connectionpool.py", line 96, in test_bytes_host2
    c.is_same_host(b"http://google.com")
  File "/Users/runner/work/urllib3/urllib3/.nox/test-3-6/lib/python3.6/site-packages/urllib3/connectionpool.py", line 498, in is_same_host
    if url.startswith("/"):
TypeError: startswith first arg must be bytes or a tuple of bytes, not str
___________________________ TestUtil.test_bytes_url ____________________________
Traceback (most recent call last):
  File "/Users/runner/work/urllib3/urllib3/test/test_util.py", line 169, in test_bytes_url
    parse_url(b"http://google.com")
  File "/Users/runner/work/urllib3/urllib3/.nox/test-3-6/lib/python3.6/site-packages/urllib3/util/url.py", line 354, in parse_url
    if not SCHEME_RE.search(url):
TypeError: cannot use a string pattern on a bytes-like object

Any other ideas? We could try testing _normalize_host directly, but looks like other _internal functions are not tested directly?

[sethmlarson]

@crwilcox Just confirming so it's explicit, you're in favor of removing contrib/_app_engine_environ.py?

@hugovk If it's unreachable from public APIs we can remove it. My guess is the branch was only reachable from Python 2 because of str==bytes. I don't think we support bytes as host in Python 3.

[crwilcox]

@sethmlarson 👍

[hugovk]

Updated, coverage is 100% and ready for review!

[pquentin]

@hugovk Fantastic! Can I ask you to open a new pull request for Google App Engine removal? This will make this one more palatable and I wouldn't want GAE removal to be in the same commit.

Sorry that I'm asking you to do more work here! Hopefully your clean separation of commits should make it easy.

@crwilcox Thanks a lot for your help here! We'll continue to support GAE in 1.x but it's good that we won't need to in 2.x.

[sethmlarson]

Merged successfully :) Thanks @hugovk!!

[graingert]

@hugovk you should consider running pyupgrade in CI with pre-commit

[hugovk]

Yep, I do in my projects :)

What do the urllib3 maintainers think?

[pquentin]

I'd be happy to try using that! Also, I know @sethmlarson agreed to explore pre-commit itself, so a good start would be to open a PR with pyupgrade as a pre-commit hook checked in GitHub Actions? Later on we could put flake8, isort and black checks in pre-commit too.

[hugovk]

Please see PR #2079.

[asottile]

fwiw you may want to consider https://pre-commit.ci (open, secret alpha, should be non-secret later today if I get the docs completed -- https://results.pre-commit.ci) -- it provides several advantages over github actions