[PATCH 1/5] models: Make use of aggregates
dja at axtens.net
Thu Oct 13 10:13:31 AEDT 2016
> - if len(orders) > 0:
> - max_order = orders['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.
> - # 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?
> - 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.
Apart from that, looks good to me.
> def public_url(self):
> if not self.public:
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
More information about the Patchwork