prowdsponsor / esqueleto

Bare bones, type-safe EDSL for SQL queries on persistent backends.
http://hackage.haskell.org/package/esqueleto
BSD 3-Clause "New" or "Revised" License
178 stars 51 forks source link

distinctOnOrderBy takes a 2nd argument for extra order columns #103

Closed gregwebs closed 9 years ago

gregwebs commented 9 years ago

a hanging list is a low overhead

The first query of ours that I looked at had 2 columns in Distinct On and 3 in Order By, so the first 2 would go in the first argument and the 3rd would go in the 2nd. That API would seem to match the semantics

The DISTINCT ON expression(s) must match the leftmost ORDER BY expression(s).

My other option would be to put the 3rd column in a new orderBy clause, but the semantics of multiple orderBy clauses are not clear. Perhaps if it just needs to be well documented that I can add the third column that way.

Another option instead of a 2nd list argument would be something combinator based

(distinctOnOrderBy [asc foo, desc bar] `extraOrderBy` [desc quux]) $ do

But I think there is too much orderBy stuff then.

Note: if you merged this PR you could do a minor version release and just deprecate 2.2.4

meteficha commented 9 years ago

I'm opposed to adding another argument to distinctOnOrderBy. Besides being more difficult to read, both parameters would have the same types.

I'm not opposed to extraOrderBy per se, but I think it's unnecessary. Esqueleto has always supported concatenating all kinds of clauses as this makes it a lot easier to compose queries. If you're worried about it, what about being more explicit on the docs and having a new test to avoid regressions?

gregwebs commented 9 years ago

If you're worried about it, what about being more explicit on the docs and having a new test to avoid regressions?

Yes, that would work