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

Revision history for this message
Danny Tamez (zematynnad) wrote :

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

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.

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

review: Needs Fixing

« Back to merge proposal