Dear GvR,

New code review comments by GvR have been published.
Please go to http://codereview.appspot.com/2827 to read them.

Message:
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.

Details:

http://codereview.appspot.com/2827/diff/1/2
File Doc/library/urllib.parse.rst (right):

http://codereview.appspot.com/2827/diff/1/2#newcode198
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.

http://codereview.appspot.com/2827/diff/1/2#newcode215
Line 215: An alias for :func:`quote`, intended for use with a :class:`bytes`
object
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.

http://codereview.appspot.com/2827/diff/1/2#newcode223
Line 223: Replace ``%xx`` escapes by their single-character equivalent.
Should add what the argument type is -- I vote for str or bytes/bytearray.

http://codereview.appspot.com/2827/diff/1/2#newcode242
Line 242: .. function:: unquote_to_bytes(string)
Again, add what the argument type is.

http://codereview.appspot.com/2827/diff/1/4
File Lib/email/utils.py (right):

http://codereview.appspot.com/2827/diff/1/4#newcode224
Line 224: except:
An unqualified except clause is unacceptable here.  Why do you need this  
anyway?

http://codereview.appspot.com/2827/diff/1/5
File Lib/test/test_http_cookiejar.py (right):

http://codereview.appspot.com/2827/diff/1/5#newcode1450
Line 1450: "%3c%3c%0Anew%C3%A5/%C3%A5",
I'm guessing this test broke otherwise?  Given that this references an RFC,  
is
it correct to just fix it this way?

http://codereview.appspot.com/2827/diff/1/3
File Lib/urllib/parse.py (right):

http://codereview.appspot.com/2827/diff/1/3#newcode10
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.

http://codereview.appspot.com/2827/diff/1/3#newcode265
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  
and
%Aa?  (Even though the old code had the same problem.)

http://codereview.appspot.com/2827/diff/1/3#newcode283
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  
uses
single quotes, so lets stick to that (except docstrings always use double
quotes).

And more: what should a None value for encoding or errors mean?  IMO it  
should
mean "use the default".

http://codereview.appspot.com/2827/diff/1/3#newcode382
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.

http://codereview.appspot.com/2827/diff/1/3#newcode399
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.

Sincerely,

   Your friendly code review daemon (http://codereview.appspot.com/).