bpo-16285: Update urllib quoting to RFC 3986 #173

rtnpro · 2017-02-19T08:55:10Z

This pull request enhances urllib quoting from RFC 2396 to RFC 3986.

This is an effort to merge initial efforts at http://bugs.python.org/file34950/0be3805cade1.diff in http://bugs.python.org/issue16285.

the-knights-who-say-ni · 2017-02-19T08:55:11Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

ncoghlan

The change looks basically OK, but will need test updates, and probably docs updates as well.

ncoghlan · 2017-02-19T11:30:23Z

Doc/library/urllib.parse.rst

   function is intended for quoting the path section of URL. The optional *safe*
   parameter specifies additional ASCII characters that should not be quoted
   --- its default value is ``'/'``.

   *string* may be either a :class:`str` or a :class:`bytes`.

+   Python 3.7 updates from using RFC 2396 to RFC 3986 to quote URL strings.
+   Now, "~" is included in the set of reserved characters.


This should use a .. versionchanged: 3.7 note (there are several examples for that earlier in the file).

ncoghlan · 2017-02-19T11:30:59Z

Lib/test/test_urllib.py

@@ -761,7 +761,7 @@ def test_never_quote(self):
        do_not_quote = '' .join(["ABCDEFGHIJKLMNOPQRSTUVWXYZ",
                                 "abcdefghijklmnopqrstuvwxyz",
                                 "0123456789",
-                                 "_.-"])
+                                 "_.-~"])


And this is the test case update I missed in my earlier review :)

rtnpro · 2017-02-22T06:31:24Z

@ncoghlan Is there anything else that needs to be updated in this PR?

ncoghlan · 2017-02-23T15:00:33Z

Doc/library/urllib.parse.rst

   function is intended for quoting the path section of URL. The optional *safe*
   parameter specifies additional ASCII characters that should not be quoted
   --- its default value is ``'/'``.

   *string* may be either a :class:`str` or a :class:`bytes`.

+   .. versionchanged:: 3.7
+      Moving from RFC 2396 to RFC 3986 for quoting URL strings. Now, "~" is


We typically write these notes in past tense, so a slight tweak here:

Moved from RFC ...

And in the 2nd sentence:

"~" is now included...

ncoghlan · 2017-02-23T15:02:32Z

Lib/urllib/parse.py


    Each of these characters is reserved in some component of a URL,
    but not necessarily in all of them.

+    Python 3.7 updates from using RFC 2396 to RFC 3986 to quote URL strings.
+    Now, "~" is included in the set of reserved characters.
+


Sorry I missed this one earlier: there's no need to have the version change info in the docstring, so the change in the RFC reference and the addition of ~ to the set of reserved characters is sufficient here.

it's also wrong as stated above.

ncoghlan · 2017-02-23T15:03:50Z

@rtnpro Almost! Just a couple of small comments on the docs and docstring updates.

rtnpro · 2017-02-24T04:34:06Z

@ncoghlan versionchanged documentation fixed by 6ce8001

ncoghlan · 2017-02-24T04:54:46Z

Assigning to myself to do the final ACKS/NEWS/What's New updates before merging.

Initial work done by ctheune at http://bugs.python.org/file34950/0be3805cade1.diff.

rtnpro · 2017-02-24T09:49:37Z

Squashed commits!

ncoghlan · 2017-05-28T05:08:57Z

Added the sprint label, as this PR was submitted at the PyCon Pune 2017 core development sprint.

openandclose · 2018-11-25T19:08:36Z

I think the document and the comments are wrong here.

What's changed is the inclusion of '~' to unreserved characters.
https://tools.ietf.org/html/rfc3986#section-2.3

So the comments here are wrong.

cpython/Lib/urllib/parse.py

Lines 742 to 743 in 41439a0

    
               reserved    = ";" | "/" | "?" | ":" | "@" | "&" | "=" | "+" | 
        
                             "$" | "," | "~"

cpython/Lib/urllib/parse.py

Lines 748 to 750 in 41439a0

    
               Python 3.7 updates from using RFC 2396 to RFC 3986 to quote URL strings. 
        
               Now, "~" is included in the set of reserved characters.

41439a0#diff-01126e4d609357e876a7fd7baa62ce18

And the document here is wrong.

cpython/Doc/library/urllib.parse.rst

Lines 461 to 464 in 41439a0

    
           .. versionchanged:: 3.7 
        
              Moved from RFC 2396 to RFC 3986 for quoting URL strings. "~" is now 
        
              included in the set of reserved characters.

41439a0#diff-67a4980d053b561c26794c63eb5ac1de

joernhees · 2019-03-05T08:43:15Z

Being pinged about this in #2568, i can only repeat: Python never actually followed 2396 to quote, but did always quote additional characters such as () (see the old https://bugs.python.org/issue12910#msg297675). The problem is that this has been in place for so long that people will definitely rely on it.

Nevertheless, stating that quote was updated to RFC 3986 is still somewhat misleading.

joernhees · 2019-03-06T18:08:20Z

Lib/urllib/parse.py

    the following reserved characters.

    reserved    = ";" | "/" | "?" | ":" | "@" | "&" | "=" | "+" |
-                  "$" | ","
+                  "$" | "," | "~"


This is wrong: "~" is in the set of _UN_reserved chars in RFC 3986, please see https://bugs.python.org/issue12910 and its PR #2568

joernhees · 2019-03-06T18:08:27Z

Lib/urllib/parse.py


    Each of these characters is reserved in some component of a URL,
    but not necessarily in all of them.

+    Python 3.7 updates from using RFC 2396 to RFC 3986 to quote URL strings.
+    Now, "~" is included in the set of reserved characters.
+


it's also wrong as stated above.

The unused full clobber list must contain "st" too.

the-knights-who-say-ni added the CLA not signed label Feb 19, 2017

ncoghlan requested changes Feb 19, 2017

View reviewed changes

rtnpro force-pushed the fix-16285 branch 2 times, most recently from 9e93107 to 1294937 Compare February 19, 2017 11:25

ncoghlan reviewed Feb 19, 2017

View reviewed changes

rtnpro force-pushed the fix-16285 branch from 1294937 to 0b8e440 Compare February 19, 2017 11:35

Mariatta removed the CLA not signed label Feb 21, 2017

the-knights-who-say-ni added the CLA signed label Feb 21, 2017

ncoghlan reviewed Feb 23, 2017

View reviewed changes

ncoghlan approved these changes Feb 24, 2017

View reviewed changes

ncoghlan self-assigned this Feb 24, 2017

bpo-16285: Update urllib quoting to RFC 3986

41439a0

Initial work done by ctheune at http://bugs.python.org/file34950/0be3805cade1.diff.

rtnpro force-pushed the fix-16285 branch from f4e7bff to 41439a0 Compare February 24, 2017 09:48

ncoghlan added 4 commits February 25, 2017 15:30

Update ACKS

3e8ac1b

Update What's New in 3.7 guide

a4bd54b

Merge remote-tracking branch 'origin/master' into fix-16285

1881cef

Add NEWS entry

c67169c

ncoghlan merged commit 21024f0 into python:master Feb 25, 2017

ncoghlan added the sprint label May 28, 2017

openandclose mentioned this pull request Mar 2, 2019

bpo-12910: update and correct quote docstring #2568

Merged

joernhees reviewed Mar 6, 2019

View reviewed changes

akruis pushed a commit to akruis/cpython that referenced this pull request May 27, 2021

Stackless issue python#173: Fix the amd64 unix clobber list

d29d047

The unused full clobber list must contain "st" too.

Uh oh!

bpo-16285: Update urllib quoting to RFC 3986 #173

bpo-16285: Update urllib quoting to RFC 3986 #173

Uh oh!

Conversation

rtnpro commented Feb 19, 2017

Uh oh!

the-knights-who-say-ni commented Feb 19, 2017

Uh oh!

ncoghlan 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!

Choose a reason for hiding this comment

Uh oh!

rtnpro commented Feb 22, 2017

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!

ncoghlan commented Feb 23, 2017

Uh oh!

rtnpro commented Feb 24, 2017

Uh oh!

ncoghlan commented Feb 24, 2017

Uh oh!

rtnpro commented Feb 24, 2017

Uh oh!

ncoghlan commented May 28, 2017

Uh oh!

openandclose commented Nov 25, 2018

Uh oh!

joernhees commented Mar 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!