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

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Hey Aaron! Thanks for the branch.. it's looking great!

There's a small typo in your makeUsefulness() factory method ('review = self.makeUser()'), but other than that, it looks ready to land :)

If you've got time, here are a few optional things that you may (or may not) want to think about:
 * I wonder if it'd be worthwhile cutting the username out of the url:
    r'^1.0/usefulness/$
   and instead adding optional filtering via GET (/1.0/usefulness/?username=aaronp)? To make it easier, we could just require the username GET param at the moment, but it means we can extend the API later without adding more urls? See what you think.
 * A few long lines there... if you get a chance, just snip them to 78 chars (no problem if you don't...).
 * Great tests! I assume we'll also want to add this call to the rnr client? If so, just patch your local rnr client (parts/required-isd-branches/rnrclient/rnrclient.py) and add a basic test to test_rnrclient() that exercises the new method. Update the description of this MP with a link to the patch for rnrclient and we can make sure it lands at the same time.

« Back to merge proposal