dabapps / django-readers

A lightweight function-oriented toolkit for better organisation of business logic and efficient selection and projection of data in Django projects.
https://www.django-readers.org
BSD 2-Clause "Simplified" License
183 stars 7 forks source link

Should we change the default for the `distinct` argument to the `count` and `has` pair functions? #57

Closed j4mie closed 1 year ago

j4mie commented 2 years ago

The distinct argument to any Aggregate subclass (Count, Sum, Avg etc) in the ORM defaults to False:

>>> print(models.Widget.objects.only("id").annotate(name_count=Count("name")).query)
SELECT "widget"."id", COUNT("widget"."name") AS "name_count" FROM "widget" GROUP BY "widget"."id"

However, in our pairs.count and pairs.has shortcuts, we override the default to be True:

>>> prepare, project = pairs.count("name")
>>> print(prepare(models.Widget.objects.all()).query)
SELECT "widget"."id", COUNT(DISTINCT "widget"."name") AS "name_count" FROM "widget" GROUP BY "widget"."id"

This was done because the DISTINCT version is arguably less surprising and gives the correct answer more of the time, ie it usually expresses what the developer intended, especially when multiple aggregations are combined.

However, to a developer who already knows how these aggregates work in Django, it's probably much more surprising that the defaults are reversed in django-readers.

So: should we change the default here and bring django-readers back into alignment with Django itself? This would also simplify the implementation of pairs.count etc (see #56). However, it would be a backwards-incompatible change and would therefore require a major version bump 😬