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

gh-82151: Improve docs for urllib.parse by idomic · Pull Request #18631 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-82151: Improve docs for urllib.parse - #18631

Closed
idomic wants to merge 16 commits into
python:mainfrom
idomic:bpo-37970-doc-fix
ConversationCommits (16)ChecksFiles changed
Closed

gh-82151: Improve docs for urllib.parse#18631
idomic wants to merge 16 commits into
python:mainfrom
idomic:bpo-37970-doc-fix

Conversation

idomic commented Feb 23, 2020 •
edited by AlexWaygood
Loading

Copy link
Copy Markdown
Contributor

bedevere-bot added docs Documentation in the Doc dir awaiting review labels Feb 23, 2020

idomic commented Mar 1, 2020

Copy link
Copy Markdown
Contributor Author

Tagging @taleinat

csabella requested review from taleinat and zware March 11, 2020 00:35
taleinat suggested changes Mar 12, 2020
View reviewed changes

taleinat left a comment

Copy link
Copy Markdown
Contributor

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

The fix here is good and important.

However, this does not address the central issue brought up in bpo-37970, regarding this line of the docs being misleading, since the parts of the netloc are available as additional attributes on the returned named-tuple object:

The components are not broken up in smaller parts (for example, the network location is a single string)

bedevere-bot commented Mar 12, 2020

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.

idomic commented Mar 15, 2020

Copy link
Copy Markdown
Contributor Author

The fix here is good and important.

However, this does not address the central issue brought up in bpo-37970, regarding this line of the docs being misleading, since the parts of the netloc are available as additional attributes on the returned named-tuple object:

The components are not broken up in smaller parts (for example, the network location is a single string)

You're right, don't know why I missed this part, fixed for both urlsplit and parse.

idomic commented Mar 15, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

bedevere-bot commented Mar 15, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

bedevere-bot requested a review from taleinat March 15, 2020 14:13
taleinat suggested changes Mar 18, 2020
View reviewed changes

taleinat left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

I think this change to the documentation is still not clear enough. Writing "The components are broken up into smaller parts (for example, ...)" is confusing, since for example the query is not broken down into its parts.

What really happens is that the parts as described in the beginning are used as-is for the named-tuple fields. Additionally, only the netloc is broken down into username, password, hostname, and port, and those are added as additional attributes on the returned object. Further, these extra attributes are only accessible by name, but not by index - they are not technically part of the named-tuple's fields.

We need to convey this clearly and concisely, which needs some more thought. The currently suggested wording doesn't achieve this.

bedevere-bot commented Mar 18, 2020

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.

idomic commented Mar 22, 2020

Copy link
Copy Markdown
Contributor Author

@taleinat I've changed one description, once we'll decide on the format I'll clone for the other function

idomic commented May 17, 2020

Copy link
Copy Markdown
Contributor Author

@taleinat ping on that

taleinat commented May 19, 2020

Copy link
Copy Markdown
Contributor

Apologies for the delay, @idomic.

What you've written is better, I like the direction :)

I suggest omitting the first sentence, which I find redundant, changing the order of the sentences, and changing "expanded" to "decoded":

The delimiters as shown above
are not part of the result, except for a leading slash in the path component, which is
retained if present.

Additionally, the netloc item is broken down into: username, password, hostname, and port. These are added as additional attributes of the returned object.

% escapes are not decoded.

For example: ...

What do you think?

idomic commented May 24, 2020

Copy link
Copy Markdown
Contributor Author

@taleinat Agree, I like the decoded change, and the first paragraph.
What do you think about:

Additionally, the netloc property is broken down into these additional attributes in the returned object: username, password, hostname, and port.

taleinat previously requested changes Jun 28, 2020
View reviewed changes
Comment thread Doc/library/urllib.parse.rst Outdated

idomic commented Jun 28, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

bedevere-bot commented Jun 28, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

bedevere-bot requested a review from taleinat June 28, 2020 14:24
faraz-mtk pushed a commit to MediaTek-Labs/cpython that referenced this pull request Nov 12, 2021
Issue python#18631: MINGW: setup msvcrt and _winapi modules
6eda102
AlexWaygood changed the title bpo-37970: Added documentation fixes bpo-37970: Improve docs for urllib3.parse Oct 30, 2022
AlexWaygood changed the title bpo-37970: Improve docs for urllib3.parse gh-82151: Improve docs for urllib3.parse Oct 30, 2022
zware mentioned this pull request Nov 5, 2022
Closed
mtvx reviewed Jul 14, 2024
View reviewed changes

mtvx left a comment

Copy link
Copy Markdown

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

Looking into this years old PR.

The function docstrings were adjusted in #16458. What remains are updates to the reference docs (this PR).

As I see it, the author has done all the fixes requested.

Added a few comments/suggestions. But in general, it looks good to me.

Maybe @zware @taleinat @encukou could have a look? @idomic you still around to potentiall adjust?

(Looking into this as part of EuroPython 2024 sprint - I'm just a user, not maintainer, fyi.)

Comment thread Doc/library/urllib.parse.rst
Comment thread Doc/library/urllib.parse.rst
Comment thread Doc/library/urllib.parse.rst Outdated
willingc reviewed Nov 1, 2024
View reviewed changes

willingc left a comment

Copy link
Copy Markdown
Contributor

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

Let's reboot this PR and move into a mergeable state.

Comment thread Doc/library/urllib.parse.rst Outdated
Comment thread Doc/library/urllib.parse.rst Outdated
willingc dismissed taleinat’s stale review November 1, 2024 23:00

Suggested changes were applied.

zware changed the title gh-82151: Improve docs for urllib3.parse gh-82151: Improve docs for urllib.parse Nov 5, 2024

StanFromIreland commented Jan 26, 2025

Copy link
Copy Markdown
Member

Hello, the original creator has not been active for quite some time. I can open a PR for #82151 myself if @idomic doesn't mind.

idomic commented Jan 26, 2025

Copy link
Copy Markdown
Contributor Author

Go ahead!

github-actions Bot commented Apr 7, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 7, 2026
pranavchoudhary-tech mentioned this pull request Jul 3, 2026
Merged

cmaloney commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR @idomic! Continuation of this work got merged in #152985

cmaloney closed this Jul 7, 2026
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

willingc willingc left review comments
zware Awaiting requested review from zware
taleinat Awaiting requested review from taleinat
+1 more reviewer
mtvx mtvx left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting changes docs Documentation in the Doc dir skip news stale Stale PR or inactive for long period of time.

Projects

Docs PRs
Status: Todo

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL