GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-69152: add method get_proxy_response_headers to HTTPConnection class by nametkin · Pull Request #104248 · python/cpython

Conversation

@nametkin

@nametkin nametkin commented May 6, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Correction according to this comment: #26152 (comment)

  • Issue: Add tunnel CONNECT response headers to httplib / http.client #69152

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot mentioned this pull request May 6, 2023
Add tunnel CONNECT response headers to httplib / http.client #69152
Closed
@bedevere-bot bedevere-bot added the awaiting review label May 6, 2023
@nametkin
nametkin force-pushed the new branch 3 times, most recently from 5cc3ca9 to 2a60f7b Compare May 6, 2023 20:50
@nametkin
add method get_proxy_response_headers
d0d9349
@nametkin
nametkin force-pushed the new branch 2 times, most recently from d493391 to 383ad4d Compare May 8, 2023 11:54
@nametkin

nametkin commented May 8, 2023

Copy link
Copy Markdown
Contributor Author

@gpshead, please look at this PR. Did I understand your idea correctly?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nametkin and others added 3 commits May 9, 2023 13:51
@nametkin
add method get_proxy_response_headers
f39feb9
@blurb-it @nametkin
📜🤖 Added by blurb_it.
0be0d47
@nametkin
Delete trailing whitespaces
cdff360
@nametkin
nametkin force-pushed the new branch from 2d7ea40 to cdff360 Compare May 9, 2023 10:51
@gpshead gpshead self-assigned this May 9, 2023
gpshead
gpshead requested changes May 9, 2023
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

yep, this is what I had in mind. Thanks for the followup PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2023-05-08-11-52-56.gh-issue-69152.tlH-q1.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/http/client.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels May 9, 2023
@bedevere-bot

bedevere-bot commented May 9, 2023

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nametkin and others added 5 commits May 10, 2023 11:51
@nametkin
update docs and news
7288cf6
@nametkin
merge commit
f49b56c
@nametkin
Merge branch 'python:main' into new
b46e44f
@nametkin
delete Misc/NEWS.d/next/Library/2023-05-08-11-52-56.gh-issue-69152.tl…
2dfc326 
…H-q1.rst
@nametkin
Merge branch 'new' of https://github.com/OneMoreZanuda/cpython into new
70ad871
@nametkin

nametkin commented May 10, 2023

Copy link
Copy Markdown
Contributor Author

@gpshead, thank you for review.
I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels May 10, 2023
@bedevere-bot

bedevere-bot commented May 10, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from gpshead May 10, 2023 09:42
nametkin added 2 commits May 10, 2023 21:25
@nametkin
Merge branch 'main' into new
03a5887
@nametkin
Merge branch 'main' into new
a7af79d
nametkin added 8 commits May 11, 2023 07:24
@nametkin
Merge branch 'main' into new
f1410b5
@nametkin
Merge branch 'main' into new
b49ec81
@nametkin
Merge branch 'main' into new
c0f50b9
@nametkin
Merge branch 'main' into new
fb77b60
@nametkin
Merge branch 'main' into new
9023028
@nametkin
Merge branch 'main' into new
63da7f3
@nametkin
Merge branch 'main' into new
6101344
@nametkin
Merge branch 'main' into new
5625bfa
@arhadthedev

arhadthedev commented May 15, 2023

Copy link
Copy Markdown
Member

The PR will be merged after the feature freezing 3.12b1 is published next week. Currently core devs implement remaining PEPs and fix bugs introduced by them.
nametkin reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
reworded news and add ReST formatting.
a21af19
gpshead
gpshead approved these changes May 16, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 16, 2023
@gpshead
gpshead enabled auto-merge (squash) May 16, 2023 05:57
@gpshead
gpshead merged commit 85ec192 into python:main May 16, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label May 16, 2023
@nametkin
nametkin deleted the new branch May 16, 2023 07:19
carljm added a commit to carljm/cpython that referenced this pull request May 16, 2023
@carljm
Merge branch 'main' into supermon
a6dd626 
* main:
  pythonGH-104510: Fix refleaks in `_io` base types (python#104516)
  pythongh-104539: Fix indentation error in logging.config.rst (python#104545)
  pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543)
  pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538)
  pythongh-64595: Fix write file logic in Argument Clinic (python#104507)
  pythongh-104523: Inline minimal PGO rules (python#104524)
  pythongh-103861: Fix Zip64 extensions not being properly applied in some cases (python#103863)
  pythongh-69152: add method get_proxy_response_headers to HTTPConnection class (python#104248)
  pythongh-103763: Implement PEP 695 (python#103764)
  pythongh-104461: Run tkinter test_configure_screen on X11 only (pythonGH-104462)
  pythongh-104469: Convert _testcapi/watchers.c to use Argument Clinic (python#104503)
  pythongh-104482: Fix error handling bugs in ast.c (python#104483)
  pythongh-104341: Adjust tstate_must_exit() to Respect Interpreter Finalization (pythongh-104437)
  pythonGH-102613: Fix recursion error from `pathlib.Path.glob()` (pythonGH-104373)
@sobolevn

sobolevn commented Jun 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Sorry for being late for the review, but I would like to note that get_proxy_response_headers now is not consistent in its return type. If there are headers, it returns HTTPMessage class (which has this mro: (<class 'http.client.HTTPMessage'>, <class 'email.message.Message'>, <class 'object'>)), but if there are no headers it returns {}. Why?

I think that the source of this confusion is that email.message.Message behaves like a Mapping, but I still think that we should change the default value to None before it is too late.

This way we can say that this method returns Optional[HTTPMessage] and not HTTPMessage | dict[str, str] (or Mapping[str, str], which will erase a lot of useful methods).

I will open a new PR for this.
nametkin, arhadthedev, and gpshead reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jun 10, 2023
Change the default return value of HTTPConnection.get_proxy_response_headers from {} to None #105626
Closed
gh-105626: Change the default return value of HTTPConnection.get_proxy_response_headers #105628
Merged
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@nametkin @bedevere-bot @arhadthedev @sobolevn @gpshead
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.