Skip to content

Conversation

@idomic
Copy link
Contributor

@idomic idomic commented Feb 23, 2020

@idomic
Copy link
Contributor Author

idomic commented Mar 1, 2020

Tagging @taleinat

@csabella csabella requested review from taleinat and zware March 11, 2020 00:35
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link
Contributor Author

idomic commented Mar 15, 2020

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
Copy link
Contributor Author

idomic commented Mar 15, 2020

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from taleinat March 15, 2020 14:13
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link
Contributor Author

idomic commented Mar 22, 2020

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

@idomic
Copy link
Contributor Author

idomic commented May 17, 2020

@taleinat ping on that

@taleinat
Copy link
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
Copy link
Contributor Author

idomic commented May 24, 2020

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

@csabella csabella requested a review from taleinat June 22, 2020 23:44
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

Two small comments.

Also, please make sure to wrap the lines at 80 characters, in keeping with (the style guide)[https://devguide.python.org/documenting/#style-guide].

@bedevere-bot
Copy link

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
Copy link
Contributor Author

idomic commented Jun 28, 2020

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from taleinat June 28, 2020 14:24
farazs-github pushed a commit to MediaTek-Labs/cpython that referenced this pull request Nov 12, 2021
@AlexWaygood AlexWaygood changed the title bpo-37970: Added documentation fixes bpo-37970: Improve docs for urllib3.parse Oct 30, 2022
@AlexWaygood AlexWaygood changed the title bpo-37970: Improve docs for urllib3.parse gh-82151: Improve docs for urllib3.parse Oct 30, 2022
Copy link

@tuukkamustonen tuukkamustonen left a comment

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

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

@willingc willingc dismissed taleinat’s stale review November 1, 2024 23:00

Suggested changes were applied.

@zware zware changed the title gh-82151: Improve docs for urllib3.parse gh-82151: Improve docs for urllib.parse Nov 5, 2024
@StanFromIreland
Copy link
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
Copy link
Contributor Author

idomic commented Jan 26, 2025

Go ahead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes docs Documentation in the Doc dir skip news

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

9 participants