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

idomic · 2019-09-28T10:51:45Z

https://bugs.python.org/issue37970

idomic · 2019-10-03T11:25:01Z

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

Ido

zware · 2019-10-03T16:11:20Z

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.

idomic · 2019-12-03T17:58:14Z

@zware fixed all of the above 👍

zware

Still need to update urlparse as well.

Lib/urllib/parse.py

zware · 2019-12-03T21:09:58Z

Misc/NEWS.d/next/Library/2019-12-03-17-38-02.bpo-37970.f7Vrwm.rst

@@ -0,0 +1 @@
+Changed documentation for: Lib/urllib/parse.py at the split function


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

bedevere-bot · 2019-12-03T21:11:15Z

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.

csabella · 2020-01-26T01:06:26Z

@idomic, please address the review comments. Thanks!

idomic · 2020-02-02T14:16:45Z

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

idomic · 2020-02-08T21:50:59Z

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 · 2020-02-08T22:23:41Z

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.

idomic · 2020-02-11T03:54:53Z

Hey @taleinat adding you to accelerate this PR

taleinat · 2020-02-11T12:32:39Z

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.

idomic · 2020-02-11T13:22:57Z

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

taleinat · 2020-02-11T14:16:42Z

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

taleinat

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.

taleinat · 2020-02-11T14:31:14Z

Lib/urllib/parse.py

+    The scheme, netloc, path, params, query, fragment
+    of netloc can also be accessed as attributes of the
+    ParseResult object.


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

taleinat · 2020-02-11T14:32:14Z

Lib/urllib/parse.py

@@ -364,11 +364,19 @@ def _fix_result_transcoding():
 del _fix_result_transcoding

 def urlparse(url, scheme='', allow_fragments=True):
-    """Parse a URL into 6 components:
+    """Parse URL and return a ParseResult.
+    SplitResult is a named 6-tuple composed of:


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

taleinat · 2020-02-11T14:35:38Z

Lib/urllib/parse.py

@@ -364,11 +364,19 @@ def _fix_result_transcoding():
 del _fix_result_transcoding

 def urlparse(url, scheme='', allow_fragments=True):
-    """Parse a URL into 6 components:
+    """Parse URL and return a ParseResult.


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

taleinat · 2020-02-11T14:38:52Z

Misc/NEWS.d/next/Documentation/2020-02-11-13-22-06.bpo-379070.aaZnkk.rst

@@ -0,0 +1 @@
+Changed documentation for: Lib/urllib/parse.py at the split and parse functions


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.

idomic · 2020-02-12T01:07:02Z

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

idomic · 2020-02-12T01:07:07Z

I have made the requested changes; please review again

bedevere-bot · 2020-02-12T01:07:10Z

Thanks for making the requested changes!

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

taleinat · 2020-02-12T11:12:42Z

Lib/urllib/parse.py

-    Note that we don't break the components up in smaller bits
-    (e.g. netloc is a single string) and we don't expand % escapes."""
+
+    The result is a ParseResult/SplitResult object, which is a named 6-tuple with


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.

taleinat · 2020-02-12T11:14:23Z

Lib/urllib/parse.py

-    Note that we don't break the components up in smaller bits
-    (e.g. netloc is a single string) and we don't expand % escapes."""
+
+    The result is a SplitResult object, which is a named 5-tuple with


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

Lib/urllib/parse.py

taleinat

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

(Also please remove the NEWS entry file.)

taleinat · 2020-02-13T21:39:37Z

Lib/urllib/parse.py

-    (e.g. netloc is a single string) and we don't expand % escapes."""
+
+    The result is a ParseResult or ParseResultBytes object,
+    depending on the type of the URL parameter.


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.

taleinat · 2020-02-13T21:41:54Z

Lib/urllib/parse.py

+    The result is a ParseResult or ParseResultBytes object,
+    depending on the type of the URL parameter.
+
+    The resulting object is a named 6-tuple with fields


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

taleinat · 2020-02-13T21:43:21Z

Misc/NEWS.d/next/Documentation/2020-02-11-13-22-06.bpo-379070.aaZnkk.rst

@@ -0,0 +1 @@
+Changed documentation for: Lib/urllib/parse.py at the split and parse functions


@idomic, please remove this file.

bedevere-bot · 2020-02-13T21:44:05Z

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 · 2020-02-13T22:10:46Z

I have made the requested changes; please review again

bedevere-bot · 2020-02-13T22:10:49Z

Thanks for making the requested changes!

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

idomic · 2020-02-14T13:10:40Z

@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

taleinat · 2020-02-16T16:00:38Z

@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

idomic · 2020-02-16T16:57:34Z

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

bedevere-bot · 2020-02-16T16:57:37Z

Thanks for making the requested changes!

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

taleinat · 2020-02-16T18:06:15Z

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

taleinat · 2020-02-16T19:19:42Z

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!

idomic · 2020-02-16T20:48:56Z

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

tuukkamustonen mentioned this pull request Jul 14, 2024

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

Open

zware mentioned this pull request Oct 24, 2024

urllib.parse docstrings incomplete #82151

Open

		@@ -0,0 +1 @@
		Changed documentation for: Lib/urllib/parse.py at the split function

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

Uh oh!

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

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

Uh oh!

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!

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