Code review comment for lp:~dooferlad/linaro-ci-dashboard/add_api_tastypie

On 4 February 2013 16:09, Stevan Radaković <email address hidden> wrote:
> Hey James,
>
> Nice work rly.. I've managed to run everything pretty much smooth, tests worked, and the code looks good as well, disregarding the problem with django version for unit testing, this is really unfortunate dependency :/
>
> First thing that kinda bothers me is that we require 'v1/' in API links.
> I read the tastypie docs a bit but couldn't get quick answer if this is
> easy to remove or not.. If there's any special reason to keep it tho,
> I'd like to hear it.

I haven't looked into that much, but versioning is probably a good
idea. so incompatible changes can be rolled out without breaking old
clients (well, we will phase out old clients eventually). I don't care
what the version string is though. I like Milo's comment about just
having a number or "dev". My vote is for major.minor version as stable
API versions and dev, which will always point to the latest API.

> Second thing is just a minor code block in tests that stung me in the eye, but nothing special:
>
> 280 + # One loop is of type AndroidLoop, the other ModelBase. Find which is
> 281 + # which.
> 282 + if json[0]['type'] == 'AndroidLoop':
> 283 + android_loop_index = 0
> 284 + base_loop_index = 1
> 285 + else:
> 286 + android_loop_index = 1
> 287 + base_loop_index = 0
> 288 +
> 289 + # Now test the contents of the loop data
> 290 + self.assertEqual(json[android_loop_index]['name'],
> 291 + self.android_loop.name)
> 292 + self.assertEqual(json[base_loop_index]['name'],
> 293 + self.loop.name)
>
> Why not use sets here, like
>
> api_test_names = {json[0]['name'], json[1]['name']}
> test_names = {self.android_loop.name, self.loop.name}
> self.assertEqual(api_test_names, test_names)

I like your version. I clearly ran out of Python at that point :-)

Thanks,

--
James Tunnicliffe