bpo-12910: update and correct quote docstring #2568

joernhees · 2017-07-04T15:11:43Z

Fixes some mistakes and misleadings in the quote function docstring:

reserved chars are never actually used by quote code, unreserved chars are
reserved chars in comment were wrong and incomplete
mentioned that use-case is not minimal quoting wrt. RFC, but cautious quoting

see discussion in https://bugs.python.org/issue12910

the-knights-who-say-ni · 2017-07-04T15:11:45Z

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 our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

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

Fixes some mistakes and misleadings in the quote function docstring: - reserved chars are never actually used by quote code, unreserved chars are - reserved chars were wrong and incomplete - mentioned that use-case is not minimal quoting wrt. RFC, but cautious quoting

joernhees · 2017-07-07T13:29:24Z

@the-knights-who-say-ni re-check?

openandclose · 2019-03-02T02:32:01Z

This PR is being in 'awaiting review' state for a while.

Meanwhile the doc has been simply wrong about tilde and reserved characters.
Can someone notify someone who might be interested?

The problem seems old,
but closely related to the recent commit updating to RFC 3986.
#173

openandclose · 2019-03-02T14:33:58Z

@joernhees @brettcannon
What dou you think about just fixing the quotational parts first?

openandclose@8ee3beb
openandclose@4a6f592

joernhees · 2019-03-05T08:50:05Z

the "quotational" parts the comments in the code referring to the RFCs?

From my point of view it would make sense to update them, but not without clearly pointing out that the code does not strictly follow them for backwards compatibility reasons

openandclose · 2019-03-05T10:12:08Z

I think I understand your point.
Please see the original commit message of the above.
openandclose@db4fd2f
(I omitted these since your commit should treat them.)

One problem is that to 'strictly follow',
they should state what is the status of these characters ("%/<>^`{}).
But I couldn't find out in RFC 3986.

joernhees · 2019-03-06T18:23:01Z

Not sure if i fully understand your statement / question: the list of chars "%/<>^`{} you present covers different areas (in RFC 3986):

characters that aren't allowed in a URI at all ("<>^`{}, but also many more likeäöüß | (see RFC3987 for IRIs and fun like that))
"reserved characters" that can appear in a URI, but have special meaning (/)
% which can only be part of a URI to escape something (in other words, if % is not followed by 2 HEXDIGs then the URI is invalid)

Still i'm quite sure that discussing those and what should've been stated in the RFC in the end doesn't lead us anywhere ;)

This PR is about fixing the mistakes in the docs that might mislead people (e.g., myself). The quote function %-escapes all characters that are neither in the unreserved chars from RFC 3986 (_ALWAYS_SAFE) nor the additional chars set via the safe arg. I still think that's the best way to explain it, which is why i made this PR.

openandclose · 2019-03-06T21:46:59Z

Sorry for the confusion, I shouldn't have included '/' and '%'.
The actual characters I intended are: "<>^`{|}.

I still feel strange about the passing mention in RFC 3987,
and the fact URI spec has to refer to IRI spec for 'ascii' characters.
But let's not talk about it now.

I brought up the two commits above, since they are so simple
it's almost absurd to say 'awaiting review'.
(I'm not interested in the commits themselves, the contents are included in your commit anyway.)

orsenthil

Thank you @joernhees and @csabella

orsenthil · 2019-04-09T23:58:07Z

Give me a moment - I skipped the reference to the change in characters in the comment, and I only assumed it was grammar correction. I am looking at this again.

orsenthil · 2019-04-10T00:00:34Z

Lib/urllib/parse.py

-                  "$" | "," | "~"
+    unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
+    reserved      = gen-delims / sub-delims
+    gen-delims    = ":" / "/" / "?" / "#" / "[" / "]" / "@"


Please change the "/" character to "|" first. Even in the comments, if we preserved the previous doc, it won't intuitively break our reading. At other places, "|" is used for or condition.

Looks like it was used as per the RFC example:
https://tools.ietf.org/html/rfc3986#appendix-A

This is okay!

orsenthil · 2019-04-10T00:29:48Z

Thanks for this discussion. The documentation change makes sense.

For a long time, we had a wrong information in the doc string.
Now, this is corrected with the mention of this.

    The quote function %-escapes all characters that are neither in the
    unreserved chars ("always safe") nor the additional chars set via the
    safe arg.

So, this should cover for the usage scenarios, and I am going to ahead with +1 with this change.

Thank you!

miss-islington · 2019-04-10T00:31:21Z

Thanks @joernhees for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2019-04-10T00:31:21Z

@orsenthil: Please replace # with GH- in the commit message next time. Thanks!

Fixes some mistakes and misleadings in the quote function docstring: - reserved chars are never actually used by quote code, unreserved chars are - reserved chars were wrong and incomplete - mentioned that use-case is not minimal quoting wrt. RFC, but cautious quoting (cherry picked from commit 750d74f) Co-authored-by: Jörn Hees <[email protected]>

bedevere-bot · 2019-04-10T00:31:49Z

GH-12754 is a backport of this pull request to the 3.7 branch.

Fixes some mistakes and misleadings in the quote function docstring: - reserved chars are never actually used by quote code, unreserved chars are - reserved chars were wrong and incomplete - mentioned that use-case is not minimal quoting wrt. RFC, but cautious quoting (cherry picked from commit 750d74f) Co-authored-by: Jörn Hees <[email protected]>

csabella · 2019-04-10T12:32:16Z

Thank @joernhees for the PR and thank you @orsenthil for the review and merge!

openandclose · 2019-04-10T17:34:53Z

@orsenthil change also parse.rst.
'~' was wrongly refered as 'reserved' there.

https://github.com/python/cpython/blob/master/Doc/library/urllib.parse.rst

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

orsenthil · 2019-04-10T17:38:43Z

@openandclose - Sure. Thanks for this note.

the-knights-who-say-ni added the CLA not signed label Jul 4, 2017

joernhees force-pushed the patch-1 branch from 776a803 to 71e8132 Compare July 7, 2017 13:21

Mariatta removed the CLA not signed label Jul 7, 2017

the-knights-who-say-ni added the CLA not signed label Jul 7, 2017

Mariatta added docs Documentation in the Doc dir and removed CLA not signed labels Jul 7, 2017

the-knights-who-say-ni added the CLA signed label Jul 10, 2017

Mariatta removed the docs Documentation in the Doc dir label Jul 10, 2017

brettcannon added the awaiting review label Feb 2, 2018

joernhees mentioned this pull request Mar 5, 2019

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

Merged

csabella requested a review from orsenthil April 9, 2019 23:50

orsenthil approved these changes Apr 9, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 9, 2019

orsenthil self-requested a review April 9, 2019 23:57

orsenthil removed the awaiting merge label Apr 9, 2019

orsenthil reviewed Apr 10, 2019

View reviewed changes

orsenthil approved these changes Apr 10, 2019

View reviewed changes

bedevere-bot added the awaiting merge label Apr 10, 2019

orsenthil added skip news needs backport to 3.7 labels Apr 10, 2019

orsenthil merged commit 750d74f into python:master Apr 10, 2019

bedevere-bot removed the awaiting merge label Apr 10, 2019

bedevere-bot removed the needs backport to 3.7 label Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-12910: update and correct quote docstring #2568

bpo-12910: update and correct quote docstring #2568

joernhees commented Jul 4, 2017 •

edited

Loading

the-knights-who-say-ni commented Jul 4, 2017

joernhees commented Jul 7, 2017

openandclose commented Mar 2, 2019

openandclose commented Mar 2, 2019 •

edited

Loading

joernhees commented Mar 5, 2019

openandclose commented Mar 5, 2019 •

edited

Loading

joernhees commented Mar 6, 2019

openandclose commented Mar 6, 2019 •

edited

Loading

orsenthil left a comment •

edited

Loading

orsenthil commented Apr 9, 2019

orsenthil Apr 10, 2019

orsenthil Apr 10, 2019

orsenthil commented Apr 10, 2019

miss-islington commented Apr 10, 2019

bedevere-bot commented Apr 10, 2019

bedevere-bot commented Apr 10, 2019

csabella commented Apr 10, 2019

openandclose commented Apr 10, 2019

orsenthil commented Apr 10, 2019

bpo-12910: update and correct quote docstring #2568

bpo-12910: update and correct quote docstring #2568

Conversation

joernhees commented Jul 4, 2017 • edited Loading

the-knights-who-say-ni commented Jul 4, 2017

joernhees commented Jul 7, 2017

openandclose commented Mar 2, 2019

openandclose commented Mar 2, 2019 • edited Loading

joernhees commented Mar 5, 2019

openandclose commented Mar 5, 2019 • edited Loading

joernhees commented Mar 6, 2019

openandclose commented Mar 6, 2019 • edited Loading

orsenthil left a comment • edited Loading

Choose a reason for hiding this comment

orsenthil commented Apr 9, 2019

orsenthil Apr 10, 2019

Choose a reason for hiding this comment

orsenthil Apr 10, 2019

Choose a reason for hiding this comment

orsenthil commented Apr 10, 2019

miss-islington commented Apr 10, 2019

bedevere-bot commented Apr 10, 2019

bedevere-bot commented Apr 10, 2019

csabella commented Apr 10, 2019

openandclose commented Apr 10, 2019

orsenthil commented Apr 10, 2019

joernhees commented Jul 4, 2017 •

edited

Loading

openandclose commented Mar 2, 2019 •

edited

Loading

openandclose commented Mar 5, 2019 •

edited

Loading

openandclose commented Mar 6, 2019 •

edited

Loading

orsenthil left a comment •

edited

Loading