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.

Sign up for GitHub

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

Jump to bottom

gh-105626: Change the default return value of HTTPConnection.get_proxy_response_headers #105628

Merged
merged 1 commit into from
Jul 14, 2023
Merged

gh-105626: Change the default return value of HTTPConnection.get_proxy_response_headers #105628

merged 1 commit into from
Jul 14, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jun 10, 2023 •
edited by github-actions bot
Loading

@sobolevn sobolevn requested a review from gpshead June 10, 2023 09:21
@bedevere-bot bedevere-bot mentioned this pull request Jun 10, 2023
Closed
@sobolevn sobolevn added the needs backport to 3.12 bug and security fixes label Jun 10, 2023
@sobolevn sobolevn mentioned this pull request Jun 10, 2023
Merged
@AlexWaygood AlexWaygood self-requested a review June 10, 2023 09:42
@sobolevn

This comment was marked as resolved.

Sign in to view
@sobolevn
Copy link
Member Author

Friendly ping :)

@sobolevn sobolevn mentioned this pull request Jun 30, 2023
Merged
@sobolevn
Copy link
Member Author

@JelleZijlstra @AlexWaygood we have a little time left :)
Otherwise, this is going to be released in 3.12

JelleZijlstra
JelleZijlstra approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Does seem like None makes more sense here, but I'd prefer to get @gpshead's opinion.

As an aside it's amusing this test is still called test_httplib, after the Python 2 name of this module.

@gpshead
Copy link
Member

gpshead commented Jul 13, 2023

None feels awkward because you are then forced to check the return value before you use it rather than just blindly use a .get("field") style API. Your goal seems to be to keep the type annotation signature simple. Can we instead return an empty HTTPMessage class?

@gpshead
Copy link
Member

gpshead commented Jul 13, 2023

I marked this as a release blocker to force us to decide before 3.12rc1 so that we don't wind up with our new API needing to have a deprecation cycle and behavior change in the future (or just live with what is understandably perceived as an API wart).

@sobolevn
Copy link
Member Author

sobolevn commented Jul 14, 2023 •
edited
Loading

@gpshead yes, from this side empty HTTPMessage might seem better.

Questions:

  1. Is default policy=compat32 for new empty HTTPMessage suitable in all cases here? Or should we use a different one?
  2. Can users actually want to check this? As far as I understand CONNECT is really optional. So, this new API with None also serves as a response to a possible question "Did we have a CONNECT call here?"
  3. Is (m.get_proxy_response_headers() or {}).get('whatever') workaround good enough to not care about potential None checks here?

@gpshead
Copy link
Member

gpshead commented Jul 14, 2023

Is (m.get_proxy_response_headers() or {}).get('whatever') workaround good enough to not care about potential None checks here?

honestly... that idiom may be enough, I expect this is going to be a very rarely used API.

@gpshead gpshead merged commit 490295d into python:main Jul 14, 2023
@miss-islington
Copy link
Contributor

Thanks @sobolevn for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@sobolevn
Copy link
Member Author

Thanks everyone :)

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 14, 2023
@sobolevn @miss-islington
pythongh-105626: Change the default return value of `HTTPConnection.g…
85ea8b9 
…et_proxy_response_headers` (pythonGH-105628)

(cherry picked from commit 490295d)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@bedevere-bot
Copy link

GH-106738 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jul 14, 2023
gpshead pushed a commit that referenced this pull request Jul 14, 2023
@miss-islington @sobolevn
[3.12] gh-105626: Change the default return value of `HTTPConnection.…
e68b280 
…get_proxy_response_headers` (GH-105628) (#106738)

gh-105626: Change the default return value of `HTTPConnection.get_proxy_response_headers` (GH-105628)
(cherry picked from commit 490295d)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
kgdiem pushed a commit to kgdiem/cpython that referenced this pull request Jul 14, 2023
@sobolevn @kgdiem
pythongh-105626: Change the default return value of `HTTPConnection.g…
656c5bc 
…et_proxy_response_headers` (python#105628)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead gpshead approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood

Assignees
No one assigned
Labels
None yet
Projects
Release and Deferred blockers 🚫
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sobolevn @gpshead @miss-islington @bedevere-bot @JelleZijlstra