[PATCH 1/5] models: Make use of aggregates

[Stephen Finucane]

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
>>          else:
>> -            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():
>>              return
> 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.

Stephen