Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(431)

Issue 2827: [issue3300] urllib.quote and unquote - Unicode issues (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by GvR
Modified:
9 years, 9 months ago
Reviewers:
Antoine Pitrou, GvR, mgiuca
Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/
Visibility:
Public.

Description

http://bugs.python.org/issue3300

Patch Set 1 #

Patch Set 2 : parse.py.patch8 #

Patch Set 3 : patch 9 #

Patch Set 4 : patch 10 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+427 lines, -75 lines) Patch
Doc/library/urllib.parse.rst View 1 2 3 1 chunk +56 lines, -8 lines 0 comments Download
Lib/email/utils.py View 1 2 3 2 chunks +5 lines, -2 lines 0 comments Download
Lib/test/test_cgi.py View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
Lib/test/test_http_cookiejar.py View 1 2 3 2 chunks +4 lines, -1 line 0 comments Download
Lib/test/test_urllib.py View 1 2 3 6 chunks +227 lines, -22 lines 0 comments Download
Lib/test/test_wsgiref.py View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
Lib/urllib/parse.py View 1 2 3 3 chunks +131 lines, -42 lines 20 comments Download

Messages

Total messages: 17
GvR
Hi Matt, Here's a code review of your patch. I'm leaning more and more towards ...
10 years, 9 months ago (2008-08-06 21:39:34 UTC) #1
mgiuca
Hi Guido, Thanks very much for this very detailed review. I've replied to the comments. ...
10 years, 9 months ago (2008-08-07 12:01:31 UTC) #2
Antoine Pitrou
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: On 2008/08/07 12:01:31, mgiuca wrote: > But ...
10 years, 9 months ago (2008-08-07 12:49:11 UTC) #3
GvR
parse.py.patch8
10 years, 9 months ago (2008-08-07 16:58:56 UTC) #4
Antoine Pitrou
http://codereview.appspot.com/2827/diff/17/28 File Lib/urllib/parse.py (right): http://codereview.appspot.com/2827/diff/17/28#newcode275 Line 275: except KeyError: If this is supposed to catch ...
10 years, 9 months ago (2008-08-07 17:29:40 UTC) #5
GvR
New code review. Also please address Antoine's comments. 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: ...
10 years, 9 months ago (2008-08-07 17:38:01 UTC) #6
mgiuca
Oh dear, it seems I never published my comments. I made all of these before ...
10 years, 9 months ago (2008-08-10 05:39:34 UTC) #7
mgiuca
http://codereview.appspot.com/2827/diff/17/28 File Lib/urllib/parse.py (right): http://codereview.appspot.com/2827/diff/17/28#newcode275 Line 275: except KeyError: Good catch. No test case caught ...
10 years, 9 months ago (2008-08-10 05:53:52 UTC) #8
mgiuca
http://codereview.appspot.com/2827/diff/1/2 File Doc/library/urllib.parse.rst (right): http://codereview.appspot.com/2827/diff/1/2#newcode223 Line 223: Replace ``%xx`` escapes by their single-character equivalent. I ...
10 years, 9 months ago (2008-08-10 07:06:56 UTC) #9
mgiuca
http://codereview.appspot.com/2827/diff/17/28 File Lib/urllib/parse.py (right): http://codereview.appspot.com/2827/diff/17/28#newcode275 Line 275: except KeyError: On 2008/08/10 05:53:52, mgiuca wrote: > ...
10 years, 9 months ago (2008-08-10 07:45:01 UTC) #10
GvR
patch 9
10 years, 9 months ago (2008-08-12 18:53:45 UTC) #11
GvR
Here's finally my code review. Most stuff was already discussed in the tracker however. http://codereview.appspot.com/2827/diff/82/83 ...
10 years, 9 months ago (2008-08-13 17:50:13 UTC) #12
GvR
BTW, I also had a look at Bill's patch. The one good idea I hope ...
10 years, 9 months ago (2008-08-13 18:48:33 UTC) #13
mgiuca
Hi guys, thanks for these detailed reviews. Here are my comments to accompany patch 10. ...
10 years, 9 months ago (2008-08-14 15:28:59 UTC) #14
GvR
patch 10
10 years, 9 months ago (2008-08-18 18:37:10 UTC) #15
GvR
Hey Matt, I have a bunch of tiny nits, but there's no need for you ...
10 years, 9 months ago (2008-08-18 20:20:32 UTC) #16
GvR
10 years, 9 months ago (2008-08-18 21:45:42 UTC) #17
Submitted as r65838, with these changes.

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

http://codereview.appspot.com/2827/diff/79/103#newcode277
Line 277: if item == b'': raise ValueError
On 2008/08/18 20:20:32, GvR wrote:
> You don't need this line.  int(b'', 16) will raise ValueError for you, at no
> extra cost. ;-)

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode302
Line 302: if item == '': raise ValueError
On 2008/08/18 20:20:32, GvR wrote:
> if not item: ...

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode307
Line 307: if len(rest) == 0:
On 2008/08/18 20:20:32, GvR wrote:
> if not rest:

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode317
Line 317: if len(pct_sequence) > 0:
On 2008/08/18 20:20:32, GvR wrote:
> if pct_sequence:

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode319
Line 319: # res[-1] will always be empty if pct_sequence != []
On 2008/08/18 20:20:32, GvR wrote:
> Express this with an assert?  E.g.
> 
>   assert not res[-1]

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode332
Line 332: always_safe = frozenset(
On 2008/08/18 20:20:32, GvR wrote:
> Now that this is really a constant, why not name it with a leading underscore
> and all caps?  I.e. _ALWAYS_SAFE

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode340
Line 340: A mapping from bytes (in range(0,256)) to strings.
On 2008/08/18 20:20:32, GvR wrote:
> I'd prefer to stick to the PEP-8 docstring formatting rules.

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode381
Line 381: s and safe may be either str or bytes objects. encoding must not be
On 2008/08/18 20:20:32, GvR wrote:
> s -> string here and in the next line

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode416
Line 416: not perform string-to-bytes encoding.
On 2008/08/18 20:20:32, GvR wrote:
> It always returns an ASCII string.

Done.

http://codereview.appspot.com/2827/diff/79/103#newcode430
Line 430: return ''.join(quoter[c] for c in bs)
On 2008/08/18 20:20:32, GvR wrote:
> return ''.join(map(quoter.__getitem__, bs))

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b