On 2016-10-12 23:13, Daniel Axtens wrote:
Hi Stephen,

-        if len(orders) > 0:
-            max_order = orders[0]['order']
+        if orders and orders['order__max']:
+            max_order = orders['order__max'] + 1
-            max_order = 0
+            max_order = 1

I'm not super happy with the change from max_order = 0 to max_order = 1
etc here. That makes the variable represent the new order, not the
current max order.

Could you either:
 - change the name, or
 - revert back to using 0 and adding one at the end.

Good call. I'll do this.

-        # see if the patch is already in this bundle
-        if BundlePatch.objects.filter(bundle=self,
-                                      patch=patch).count():
+ if BundlePatch.objects.filter(bundle=self, patch=patch).exists():
I know you didn't change this line, but should this be an explicit
return None, given that below an object is returned?

They mean the same thing, and the plain 'return' is the more Pythonic approach in my experience.

-        bp = BundlePatch.objects.create(bundle=self, patch=patch,
-                                        order=max_order + 1)
-        bp.save()
-        return bp
+        return BundlePatch.objects.create(bundle=self, patch=patch,
+                                          order=max_order)
Is this guaranteed to save? My memory of the specific semantics of
create is a bit fuzzy.

Yup, it's a shortcut [1]

[1] http://stackoverflow.com/a/26672182/613428

Apart from that, looks good to me.

Cheers. There are issues with the other patch so I'll fix the one thing above in v2.

Patchwork mailing list

Reply via email to