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 Python 3.6 #2332

Closed
wants to merge 4 commits into from

Conversation

sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Jul 20, 2021

This PR only effects the v2.x development branch, v1.26.x will continue to support Python 3.6.

The reason for this PR is two-fold:

I think removing now is best to not impede development and if by some stroke of luck we complete all v2.0 deliverables in the Python 3.6 timeline we'll re-evaluate.

@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #2332 (bcdddc5) into main (e772731) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2332   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2439      2452   +13     
=========================================
+ Hits          2439      2452   +13     
Impacted Files Coverage Δ
src/urllib3/_collections.py 100.00% <100.00%> (ø)
src/urllib3/connection.py 100.00% <100.00%> (ø)
src/urllib3/util/ssl_.py 100.00% <100.00%> (ø)
src/urllib3/response.py 100.00% <0.00%> (ø)
src/urllib3/exceptions.py 100.00% <0.00%> (ø)
src/urllib3/util/retry.py 100.00% <0.00%> (ø)
src/urllib3/poolmanager.py 100.00% <0.00%> (ø)
src/urllib3/connectionpool.py 100.00% <0.00%> (ø)

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 e772731...bcdddc5. Read the comment docs.

@pquentin
Copy link
Member

Deleted my previous comment as I hadn't seen all the code, sorry.

Barring the codecov issue, this looks good but it would be nice if our type experts @haikuginger and @hramezani could take a look as types are evolving rapidly. I think @haikuginger mentioned in the past that 3.6 made things more difficult type wise, and that RLock wasn't the only issue.

And I guess mentioning @hugovk is always a good idea when you stop supporting a Python version :p

Copy link
Member

@shazow shazow left a comment

Choose a reason for hiding this comment

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

Looks sensible. I'm not intimately familiar with these details but let me know if you require my approval. :)

@haikuginger
Copy link
Contributor

This looks correct in general; I'm just going to re-acquaint myself with why the RLock compat code was 3.6-only before approving.

haikuginger
haikuginger previously approved these changes Jul 21, 2021
Copy link
Contributor

@haikuginger haikuginger left a comment

Choose a reason for hiding this comment

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

LGTM; this is exciting! Found the doc where threading was made non-optional in 3.7.

@pquentin
Copy link
Member

pquentin commented Jul 21, 2021

I'll look into the hostname_checks_common_name PyPy issue. (Its availability depends on OpenSSL, even on CPython.)

@hramezani
Copy link
Member

I think we have typing.OrderedDict in Python > 3.7.
So, I think we can typing.OrderedDict use in

_container: "OrderedDict[_KT, _VT]"

@hramezani
Copy link
Member

I think we can remove nox > Session test-3.6 was successful. and a line for 3.10 maybe from https://github.com/urllib3/urllib3/blob/5b61732f2403c378bb60b451d95ce2835f6b5485/docs/contributing.rst

@hramezani
Copy link
Member

There is an Optimized for Python 3.6+ section in https://github.com/urllib3/urllib3/blob/2ef05adfcf001c85f1f95ae3fa078d16190851b0/docs/v2-roadmap.rst
which may need some change.

It is only writeable with OpenSSL 1.1.0 and higher.
@pquentin
Copy link
Member

There's another error message that changed on PyPy, that I still need to fix, but the hostname_checks_common_name bug on our end is fixed.

@pquentin
Copy link
Member

CI is green again, we just need to address the comments from @hramezani now.

@hramezani
Copy link
Member

@SethMichaelLarson @pquentin I can address them if you want.

@pquentin
Copy link
Member

Yes, please do! You should be allowed to push to Seth's branch directly.

I also noticed another change needed in test_ssltransport.py (search for 3.7)

@hramezani
Copy link
Member

@sethmlarson It seems I don't have permission to push. I've created a new PR #2336

@hramezani
Copy link
Member

Closing in favor of #2336

@hramezani hramezani closed this Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants