Code review comment for lp:~aaronp/rnr-server/get-usefulness-by-user

On Wed, Mar 16, 2011 at 4:53 PM, Danny Tamez <email address hidden>wrote:

> Review: Needs Fixing
> Aaron - thanks for this. Everything looks good except for some minor
> issues.
> Could you make the following changes?
>

Hey Danny - this branch has already been approved and landed, but just on
your points below:

>
> Please change line 112 of src/reviewsapp/api/urls.py so that it uses
> parameterized urls for each variation rather than one url with a query
> string. We like to avoid query strings to be able to use caching.
>

The original MP that Aaron submitted was using a url like:

r'^1.0/usefulness/(?P<username>\w+)/$'

and I asked him to change this to use GET parms so that this API call could
be more general (filter not just by username, but also by reviewid etc.) in
the future (and aaron added a filter by reviewid just because it was easy),
based on my (possibly wrong?) assumption that caching was not so relevant
for this api call (I don't see 1000 requests for the usefulness objects for
the one user).

If you disagree, then we'll need to fix it as a separate branch since this
one has landed.

> Also (as long as you're resubmitting) - not required but it would be nice
> to fix some PEP8 issues:
>

Same - one of us can submit a branch fixing these. Similar to you, I didn't
want to require those kind of changes from community contributors. I think
when we have a make target (or auto-lint check) for this kind of stuff it
will be much easier for community contributors to check, but until then, I'm
happy to merge branches locally and then do the lint/PEP8 fixes as a
separate commit before landing it (I didn't do this this morning). Let me
know what you think.

-Michael

> please remove the extra blank line at 339 of
> src/reviewsapp/models/reviews.py
> please remove the blank line at the end of src/reviewsapp/api/handlers.py
> please add another blank line before the definitions of classes
> ShowUsefulnessHandler and ShowReviewsHandler.
> src/reviewsapp/tests/test_handlers.py
> please add another blank line before def of
> _get_url_kwargs_dict_for_review, before def of class
> ServerStatusHandlerTestCase, before def of class
> ShowUsefulnessHandlerTesetCase
> please add spaces around '=' when they aren't keyword assignments (lines
> 70, 72,
> remove space before ":" line 181 and after "{" at 534
> add space after ":" 205, 206, 208
> remove blank line at line 233
> please remove blank line at end of file in
> src/reviewsapp/tests/test_rnrclient.py
> and trim line 456 to 80 chars or less
>
> --
>
> https://code.launchpad.net/~aaronp/rnr-server/get-usefulness-by-user/+merge/53580
> You are reviewing the proposed merge of
> lp:~aaronp/rnr-server/get-usefulness-by-user into lp:rnr-server.
>