This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author gvanrossum
Recipients gvanrossum, janssen, jimjjewett, loewis, mgiuca, orsenthil, pitrou, thomaspinckney3
Date 2008-08-06.21:39:36
SpamBayes Score 1.30659e-08
Marked as misclassified No
Message-id <>
Dear GvR,

New code review comments by GvR have been published.
Please go to to read them.

Hi Matt,

Here's a code review of your patch.

I'm leaning more and more towards wanting this for 3.0, but I have some API
design issues and also some style nits.

I'm cross-linking this with the Python tracker issue, through the subject.

File Doc/library/urllib.parse.rst (right):
Line 198: replaced by a placeholder character.
I don't think that's a good default.  I'd rather see it default to strict --
that's what encoding translates to everywhere else.  I believe that lenient
error handling by default can cause subtle security problems too, by hiding
problem characters from validation code.
Line 215: An alias for :func:`quote`, intended for use with a :class:`bytes`
I'd rather see this as a wrapper that raises TypeError if the argument  
isn't a
bytes or bytearray instance.  Otherwise it's needless redundancy.
Line 223: Replace ``%xx`` escapes by their single-character equivalent.
Should add what the argument type is -- I vote for str or bytes/bytearray.
Line 242: .. function:: unquote_to_bytes(string)
Again, add what the argument type is.
File Lib/email/ (right):
Line 224: except:
An unqualified except clause is unacceptable here.  Why do you need this  
File Lib/test/ (right):
Line 1450: "%3c%3c%0Anew%C3%A5/%C3%A5",
I'm guessing this test broke otherwise?  Given that this references an RFC,  
it correct to just fix it this way?
File Lib/urllib/ (right):
Line 10: "urlsplit", "urlunsplit"]
Please add all the quote/unquote versions here too.
(They were there in 2.5, but somehow got dropped from 3.0.
Line 265: # Maps lowercase and uppercase variants (but not mixed case).
That sounds like a disaster.  Why would %aa and %AA be correct but not %aA  
%Aa?  (Even though the old code had the same problem.)
Line 283: def unquote(s, encoding = "utf-8", errors = "replace"):
Please no spaces around the '=' when used for an argument default (or for a
keyword arg).

Also see my comment about defaulting to 'replace' in the doc file.

Finally -- let's be consistent about quotes.  It seems most of this file  
single quotes, so lets stick to that (except docstrings always use double

And more: what should a None value for encoding or errors mean?  IMO it  
mean "use the default".
Line 382: safe = safe.encode('ascii', 'ignore')
Using errors='ignore' seems like a mistake -- it will hide errors.

I also wonder why safe should be limited to ASCII though.
Line 399: if ' ' in s:
This test means that it won't work if the input is bytes.  E.g.

urllib.parse.quote_plus(b"abc def")

raises a TypeError.


   Your friendly code review daemon (
Date User Action Args
2008-08-06 21:39:39gvanrossumsetrecipients: + loewis, jimjjewett, janssen, orsenthil, pitrou, thomaspinckney3
2008-08-06 21:39:38gvanrossumlinkissue3300 messages
2008-08-06 21:39:36gvanrossumcreate