◐ Shell
reader mode source ↗
Skip to content

gh-102327: Extend docs for "url" and "headers" parameters to HTTPConnection.request()#102328

Merged
Mariatta merged 9 commits into
python:mainfrom
davidfstr:f/http_url_clarify
May 9, 2023
Merged

gh-102327: Extend docs for "url" and "headers" parameters to HTTPConnection.request()#102328
Mariatta merged 9 commits into
python:mainfrom
davidfstr:f/http_url_clarify

Conversation

@davidfstr

@davidfstr davidfstr commented Feb 28, 2023

Copy link
Copy Markdown
Contributor

Added example on how to use the HTTPConnection object for making GET request.

Original issue: #102327

@davidfstr davidfstr force-pushed the f/http_url_clarify branch from a7cc220 to 86275f8 Compare April 4, 2023 23:07
@Mariatta

Copy link
Copy Markdown
Member

Looks good overall, I think this provides clarity to the doc.

One comment, and I'm being nitpicky here, can we use a different URL in the example, instead of xkcd? Can we use python.org as example? Or other Python-related website.

@davidfstr

Copy link
Copy Markdown
Contributor Author

Can we use python.org as example? Or other Python-related website.

Sure. I'll tentatively put in: https://docs.python.org/3/

That URL satisfies the following useful conditions:

  • is owned by Python contributors
  • uses "https" rather than "http", which promotes "https"
  • uses a non-trivial path "/3/" (rather than just "/")

@merwok

merwok commented Apr 30, 2023

Copy link
Copy Markdown
Member

What do you think of moving the doc for url higher up (so that order of doc paragraphs matches order of parameters), and not using a note markup (which grabs attention and takes readers out of normal reading flow)?

The example could be kept at the end, after the documentation of each parameter.

@davidfstr

Copy link
Copy Markdown
Contributor Author

@merwok , makes sense. I'll plan to integrate those revisions later today.

@davidfstr

Copy link
Copy Markdown
Contributor Author

Revisions integrated. Here's the updated graphical diff of the proposed changes:

Screen Shot 2023-05-02 at 9 26 26 AM

@merwok merwok 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.

Hide comment

This looks pretty great now!

Thanks for the screenshots. Note that pull requests have automatic preview websites for docs, which are even better to see results easily (although sometimes the link is a bit hidden):
https://cpython-previews--102328.org.readthedocs.build/en/102328/library/http.client.html#httpconnection-objects

@merwok

merwok commented May 2, 2023

Copy link
Copy Markdown
Member

Note for the dev who will merge: please edit the commit message to avoid reusing the whole original post here with its questions and screenshot

@davidfstr davidfstr changed the title gh-102327: Document "url" parameter to HTTPConnection.request() May 4, 2023
@merwok merwok requested review from AA-Turner and Mariatta May 4, 2023 15:23
@davidfstr

Copy link
Copy Markdown
Contributor Author

What are the next steps to move this review forward?

  • This review was previously approved by python maintainers @Mariatta and @AA-Turner , but that approval was reset when I cleaned up the PR title.
  • This review was also approved by @merwok. (Thanks for the helpful revisions!)

(1) Would it be helpful if I squashed/rebased this commit to the tip of the main branch?

(2) Do I need to summon someone with merge permissions? If so, is there a list of such folks publicly listed?

@Mariatta

Mariatta commented May 9, 2023

Copy link
Copy Markdown
Member

I will re-review later today from computer. Thanks.

@merwok

merwok commented May 9, 2023

Copy link
Copy Markdown
Member

The approval was reset by me actually, not a title change!

Note that all PRs for CPython are squash merged, so rebases are not needed. (In fact the devguide recommends against them, as force pushes do weird things for reviewers: ghost notifications, comments losing context)

@Mariatta Mariatta 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.

Hide comment

Thank you!

@miss-islington

Copy link
Copy Markdown
Contributor

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

@terryjreedy terryjreedy removed the needs backport to 3.11 only security fixes label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants