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

Drop support for EOL Python 2.6 #1429

Merged
merged 7 commits into from Aug 23, 2018
Merged

Drop support for EOL Python 2.6 #1429

merged 7 commits into from Aug 23, 2018

Conversation

@hugovk
Copy link
Contributor

@hugovk hugovk commented Aug 23, 2018

For #883.

Requests and pip have already dropped Python 2.6.

This PR removes support for it, removes redundant code, and also does some modernising to use newer Python features.

@codecov-io
Copy link

@codecov-io codecov-io commented Aug 23, 2018

Codecov Report

Merging #1429 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1429   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          21      21           
  Lines        1792    1790    -2     
======================================
- Hits         1792    1790    -2
Impacted Files Coverage Δ
src/urllib3/__init__.py 100% <ø> (ø) ⬆️
src/urllib3/_collections.py 100% <ø> (ø) ⬆️
src/urllib3/connection.py 100% <ø> (ø) ⬆️
src/urllib3/util/ssl_.py 100% <ø> (ø) ⬆️
src/urllib3/connectionpool.py 100% <100%> (ø) ⬆️
src/urllib3/request.py 100% <100%> (ø) ⬆️

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 85e376a...49c3604. Read the comment docs.

@hugovk hugovk mentioned this pull request Aug 23, 2018
Copy link
Member

@sethmlarson sethmlarson left a comment

This looks like a great start. Could you address a few of my comments?

supports_set_ciphers = ((2, 7) <= sys.version_info < (3,) or
(3, 2) <= sys.version_info)

class SSLContext(object): # Platform-specific: Python 2

This comment has been minimized.

@sethmlarson

sethmlarson Aug 23, 2018
Member

Can we add back a supports_set_ciphers = True here? Just for backwards compatibility.

This comment has been minimized.

@hugovk

hugovk Aug 23, 2018
Author Contributor

Reverted.

@@ -108,21 +104,10 @@ def __init__(self, *args, **kw):
if six.PY3: # Python 3
kw.pop('strict', None)

# Pre-set source_address in case we have an older Python like 2.6.
self.source_address = kw.get('source_address')

This comment has been minimized.

@sethmlarson

sethmlarson Aug 23, 2018
Member

I think we should keep this attribute on the HTTPConnection object, might be being used by others.

This comment has been minimized.

@hugovk

hugovk Aug 23, 2018
Author Contributor

Kept.

@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Aug 23, 2018

Could you comment on how you found the occurrences for these optimizations? Was it a manual effort or is there a tool for the unittest improvements?

@hugovk
Copy link
Contributor Author

@hugovk hugovk commented Aug 23, 2018

Sure, for the set/dict/list literals, I used the code inspection (Code > Inspect Code...) in PyCharm (they have a lightweight Community edition and also free licences for the full version for open source projects).

For the unit tests, I used the script at https://github.com/hugovk/upgrade-unittest which is just some sed commands, not a full AST parser, so you need to check the changes. There's also the https://github.com/jparise/flake8-assertive plugin, which I've just run and found some others which I've updated.

@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Aug 23, 2018

I'm approving these changes, just got to let CI pass now. :)

@sethmlarson sethmlarson merged commit cb21598 into urllib3:master Aug 23, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Aug 23, 2018

Thanks for doing this! 🎉

@nateprewitt
Copy link
Contributor

@nateprewitt nateprewitt commented Aug 23, 2018

As a quick note, we dropped support for 2.6 but haven’t merged the removal changes into master since that would be a breaking change for Requests 2.X.

We can probably go ahead and drop it, but we’ll need to make an announcement that it breaks now.

@hugovk hugovk deleted the hugovk:rm-2.6 branch Aug 23, 2018
@hugovk
Copy link
Contributor Author

@hugovk hugovk commented Aug 23, 2018

You're welcome!

@pquentin
Copy link
Member

@pquentin pquentin commented Aug 24, 2018

Thank you again @hugovk, that was a quick and high-quality PR. :)

Would you be interested in working on urllib3 async support with us?

@hugovk
Copy link
Contributor Author

@hugovk hugovk commented Aug 24, 2018

Thanks, happy to help out, but I'm not yet really familiar with async.

@sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Aug 24, 2018

Also @hugovk since you did the bulk of this work could you add a changelog entry for dropping Python 2.6 and add yourself to contributors list if you're not already there.

@hugovk
Copy link
Contributor Author

@hugovk hugovk commented Aug 24, 2018

@SethMichaelLarson Done! Please see #1431.

@pquentin
Copy link
Member

@pquentin pquentin commented Aug 25, 2018

@hugovk There are many ways to help even if you're not familiar with async yet! Removing Python 2.6 support was one of those things. :)

Another helpful thing to do is to improve the setup.py script: python-trio/trio#567 (comment). There are many things to do but they are not documented yet.

Feel free to open an issue in https://github.com/python-trio/urllib3/ or send me an email at quentin.pradet@gmail.com if anything is unclear.

But no pressure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.