bpo-37970: update and improve urlparse and urlsplit doc-strings by idomic · Pull Request #16458 · python/cpython · GitHub

https://bugs.python.org/issue37970

Hey @zware, would love to get feedback and close this PR.

Ido

Hi @idomic, sorry I haven't had time to get back to this yet. Some quick high-level feedback, though:

CI is failing, so that will need to be addressed. Without digging in and looking, the most likely culprit is trailing whitespace; run make patchcheck if you're on a *nix platform, or run the same commands that does if you're on Windows (yes, terrible UX there, sorry :( ).
There should be a NEWS entry for this, see the Details link on the bedevere/news check.
urlparse needs approximately the same treatment.
It would be good to go through the rest of the module and see if there are other similarly insufficient docstrings that could be updated while we're in there.

@zware fixed all of the above 👍

Still need to update urlparse as well.

Updated :func:`urllib.parse.urlsplit` docstring.

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, please address the review comments. Thanks!

Thanks @zware @csabella and sorry for the delay!
Fixed the fixed the review comments, let's see if the CI passes.

Thanks @zware @csabella and sorry for the delay!
Fixed the fixed the review comments, let's see if the CI passes.

Added the documentation for the parseurl function as well.
@zware @csabella looks like everything passed fine?

Codecov Report

Merging #16458 into master will increase coverage by 0.99%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #16458       +/-   ##
===========================================
+ Coverage   82.20%   83.20%    +0.99%     
===========================================
  Files        1958     1571      -387     
  Lines      589739   414730   -175009     
  Branches    44456    44456               
===========================================
- Hits       484793   345073   -139720     
+ Misses      95287    60016    -35271     
+ Partials     9659     9641       -18

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	30.00% <0.00%> (-65.00%)	⬇️
Lib/distutils/command/bdist_rpm.py	7.63% <0.00%> (-56.88%)	⬇️
Lib/test/test_urllib2net.py	76.92% <0.00%> (-13.85%)	⬇️
Lib/test/test_smtpnet.py	78.57% <0.00%> (-12.86%)	⬇️
Lib/ftplib.py	63.85% <0.00%> (-6.06%)	⬇️
Lib/test/test_ftplib.py	87.11% <0.00%> (-4.72%)	⬇️
Lib/dbm/__init__.py	66.66% <0.00%> (-4.45%)	⬇️
Tools/scripts/db2pickle.py	17.82% <0.00%> (-3.97%)	⬇️
Lib/test/test_socket.py	71.94% <0.00%> (-3.87%)	⬇️
Tools/scripts/pickle2db.py	16.98% <0.00%> (-3.78%)	⬇️
... and 428 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed6161...82b2473. Read the comment docs.

Hey @taleinat adding you to accelerate this PR

Hi @idomic,

It seems you've rebased your branch on a later commit on the master branch, and then merged that with the existing PR branch when pushing it.

For future reference, for PR branches, I highly recommend only merging rather than rebasing. If you have rebased, though, you should force-push the rebased branch (i.e. git push -f origin <branch-name>).

In this case, I suggest you (hard-) reset your branch to the commit before the merge, make sure it's okay, and then force-push that.

Thanks @taleinat Done, let's see if the CI passes

@idomic, looks good now!

For future reference: Please leave a comment with the words I have made the requested changes; please review again after a committer has requested changes; this will trigger a bot to update the state tags and notify the reviewer.

As mentioned by @zware, the suggested doc-strings don't follow the guidelines in PEP 257. In addition to my other comments, please fix their formatting.

This is implied directly by the previous sentence, since this is the major feature of named tuples. IMO it would be better to omit this extra sentence. (Same goes for urlsplit() below.)

Copy/paste error here: This returns a ParseResult object.

I find this first line less clear than the original (though it is technically correct.)

How about:

"""Parse a URL into 6 components:
<scheme>://<netloc>/<path>;<params>?<query>#<fragment>

The result is a ParseResult object, which is a named 6-tuple with
fields corresponding to the above.

...

This then followed by the later description of the other input parameters and the final note regarding % escapes not being expanded.

(The same goes for urlsplit() as well.)

This PR changes the doc-strings, not "the documentation", which is what goes up to docs.python.org.

Also, IMO this doesn't warrant a NEWS entry, so it can and should be removed. I'm adding the skip news label to allow that.

@idomic, please remove this file.

@taleinat Thanks for all the feedback, I noticed I forgot the space line after the doc-string summary.
Did took your version as it clarifies the role in less lines.

I have made the requested changes; please review again

Thanks for making the requested changes!

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

This will actually be a ParseResult or a ParseResultBytes object, depending on the type of the url parameter. It will never be a SplitResult object.

This will actually be a SplitResult or a SplitResultBytes object, depending on the type of the url parameter.

Almost done! Two more comments, relevant to both doc-strings.

(Also please remove the NEWS entry file.)

The parameter is named "url" rather than "URL", let's keep the correct spelling.

Suggested change

      
                depending on the type of the URL parameter.
          
                depending on the type of the url parameter.

How about, rather than this and the previous paragraphs being separate, something like:

"The result is a named 6-tuple with fields corresponding to the above. It is either a ParseResult or a ParseResultBytes object, depending on the type of the URL parameter."

It's shorter, changed to this version

@idomic, please remove this file.

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.

I have made the requested changes; please review again

Thanks for making the requested changes!

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

@taleinat Should I just cherry pick and create a new PR? I see some of the ci fails even after pulling from the master latest version

@idomic, please don't create a new PR, and there's no need. What you should do is:

Hard-reset your branch to the latest master branch
Make the latest appropriate changes (even copy-paste from GitHub works)
Review, making sure everything is fine
run make patchcheck (or run patchcheck.py directly on Windows)
Create a new commit with appropriate prefix: [bpo-37970](https://bugs.python.org/issue37970):
Force-push into this PR's branch

Thanks @taleinat , I have made the requested changes; please review again
Let's see how it goes!

Thanks for making the requested changes!

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

@idomic, it seems something went very wrong on your end with this PR's branch. I've gone ahead and force-pushed a fixed version, with a single commit.

If you want to check it out locally, note that you'll need to delete your local branch (or rename it to back it up) before pulling this branch.

Many thanks for the PR, @idomic, and for your patience repeatedly waiting for a review and making the requested changes!

I assure you that for changes such as this, it usually doesn't take this long nor as many iterations as it took us here. I hope this experience doesn't discourage you from continuing to contribute to the project!

@taleinat Not at all, I'm happy to learn and help!
Hopefully our next work will be more concise :)

the-knights-who-say-ni added the CLA signed label Sep 28, 2019

bedevere-bot added the awaiting review label Sep 28, 2019

zware requested changes Dec 3, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 3, 2019

csabella requested a review from zware February 9, 2020 18:41

idomic force-pushed the urlsplitDocString branch from 7e1aab6 to df49504 Compare February 11, 2020 13:20

taleinat suggested changes Feb 11, 2020

View reviewed changes

taleinat reviewed Feb 11, 2020

View reviewed changes

taleinat added the skip news label Feb 11, 2020

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 12, 2020

bedevere-bot requested a review from taleinat February 12, 2020 01:07

taleinat suggested changes Feb 12, 2020

View reviewed changes

bedevere-bot removed the awaiting change review label Feb 12, 2020

bedevere-bot requested a review from taleinat February 13, 2020 19:50

taleinat suggested changes Feb 13, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Feb 13, 2020

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 13, 2020

bedevere-bot requested a review from taleinat February 13, 2020 22:10

idomic force-pushed the urlsplitDocString branch from caef8dd to 276b695 Compare February 16, 2020 16:57

bpo-37970: update and improve urlparse and urlsplit doc-strings

82b2473

taleinat force-pushed the urlsplitDocString branch from 276b695 to 82b2473 Compare February 16, 2020 18:02

taleinat changed the title bpo-37970 Fixed urlsplit docstrings in urllib/parse.py bpo-37970: update and improve urlparse and urlsplit doc-strings Feb 16, 2020

taleinat approved these changes Feb 16, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 16, 2020

taleinat merged commit c33bdbb into python:master Feb 16, 2020

bedevere-bot removed the awaiting merge label Feb 16, 2020

idomic deleted the urlsplitDocString branch February 16, 2020 20:57

mtvx mentioned this pull request Jul 14, 2024

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

Closed

zware mentioned this pull request Oct 24, 2024

urllib.parse docstrings incomplete #82151

Closed

	depending on the type of the URL parameter.
	depending on the type of the url parameter.

bpo-37970: update and improve urlparse and urlsplit doc-strings by idomic · Pull Request #16458 · python/cpython · GitHub

Repository navigation

bpo-37970: update and improve urlparse and urlsplit doc-strings - #16458

bpo-37970: update and improve urlparse and urlsplit doc-strings#16458taleinat merged 1 commit intopython:masterpython/cpython:masterfrom idomic:urlsplitDocStringidomic/cpython:urlsplitDocStringCopy head branch name to clipboard

Conversation

idomic commented Sep 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

idomic commented Oct 3, 2019

Uh oh!

zware commented Oct 3, 2019

Uh oh!

idomic commented Dec 3, 2019

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 3, 2019

Uh oh!

csabella commented Jan 26, 2020

Uh oh!

idomic commented Feb 2, 2020

Uh oh!

idomic commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

idomic commented Feb 11, 2020

Uh oh!

taleinat commented Feb 11, 2020

Uh oh!

idomic commented Feb 11, 2020

Uh oh!

taleinat commented Feb 11, 2020

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taleinat Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idomic commented Feb 12, 2020

Uh oh!

idomic commented Feb 12, 2020

Uh oh!

bedevere-bot commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taleinat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpo-37970: update and improve urlparse and urlsplit doc-strings#16458
taleinat merged 1 commit into
python:masterfrom
idomic:urlsplitDocString

idomic commented Sep 28, 2019 •
edited
Loading

idomic commented Feb 8, 2020 •
edited
Loading

codecov Bot commented Feb 8, 2020 •
edited
Loading

taleinat Feb 11, 2020 •
edited
Loading

taleinat left a comment •
edited
Loading

taleinat commented Feb 16, 2020 •
edited by bedevere-bot
Loading

idomic commented Feb 16, 2020 •
edited
Loading