FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-105626: Change the default return value of `HTTPConnection.get_proxy_response_headers` by sobolevn · Pull Request #105628 · python/cpython · GitHub

/ cpython Public

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

Merged
gpshead merged 1 commit into
python:mainfrom
sobolevn:issue-105626
Jul 14, 2023
ConversationCommits (1)ChecksFiles changed
Merged

gh-105626: Change the default return value of HTTPConnection.get_proxy_response_headers#105628
gpshead merged 1 commit into
python:mainfrom
sobolevn:issue-105626

Conversation

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

Copy link
Copy Markdown
Member

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

This comment was marked as resolved.

Sign in to view

sobolevn commented Jun 27, 2023

Copy link
Copy Markdown
Member Author

Friendly ping :)

sobolevn mentioned this pull request Jun 30, 2023
Merged

sobolevn commented Jul 13, 2023

Copy link
Copy Markdown
Member Author

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

JelleZijlstra approved these changes Jul 13, 2023
View reviewed changes

JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 13, 2023

Copy link
Copy Markdown
Member

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 commented Jul 13, 2023

Copy link
Copy Markdown
Member

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 commented Jul 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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 commented Jul 14, 2023

Copy link
Copy Markdown
Member

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 merged commit 490295d into python:main Jul 14, 2023

miss-islington commented Jul 14, 2023

Copy link
Copy Markdown
Contributor

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

sobolevn commented Jul 14, 2023

Copy link
Copy Markdown
Member Author

Thanks everyone :)

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 14, 2023
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 commented Jul 14, 2023

Copy link
Copy Markdown

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

bedevere-bot removed the needs backport to 3.12 only security fixes label Jul 14, 2023
gpshead pushed a commit that referenced this pull request Jul 14, 2023
[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
pythongh-105626: Change the default return value of `HTTPConnection.g…
656c5bc 
…et_proxy_response_headers` (python#105628)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gpshead gpshead approved these changes
JelleZijlstra JelleZijlstra approved these changes
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

Back | FazBrowse Home | New Git URL