Code review comment for lp:~cprov/launchpad/bug-404144-unicode-ppa-searches

Revision history for this message
Celso Providelo (cprov) wrote :

On Mon, Jul 27, 2009 at 11:11 AM, Aaron Bentley<email address hidden> wrote:
> Review: Approve
>> It consists of changing query_string_parameters() to already decode variables,
>> and on the Distribution:+search view we can simply use the original form
>> QUERY_STRING, instead of rebuilding it with urlencode.
>
> As we discussed on IRC,
>
> 1. Please don't use setdefault at line 23, because we already know there's no value for that key
> 2. Please add a line break at line 28
> 3. Please clarify that get_query_string_params returns lists of unicode strings in the docstring.
> 4. Please explain that the doctest at 188 would raise an exception if the value weren't utf-8 decoded (e.g. "The 'name_filter' is decoded as UTF-8 before further processing.  If it did not, an exception would be raised.")

Thanks for your review, Aaron.

Full diff with your comments addressed goes attached.

--
Celso Providelo <email address hidden>
IRC: cprov, Jabber: <email address hidden>, Skype: cprovidelo
1024D/681B6469 C858 2652 1A6E F6A6 037B B3F7 9FF2 583E 681B 6469

« Back to merge proposal