korma / Korma

Tasty SQL for Clojure.
http://sqlkorma.com
1.48k stars 222 forks source link

order no longer accepts varargs arguments as that only hides bugs in … #292

Closed lokori closed 9 years ago

lokori commented 9 years ago

…the user code.

Specifically, the before the change, following code produces perfectly legal SQL:

(sql/select t
    (sql/order :a :ASC :b :ASC))

However, the ordering doesn't work as intended. Korma just takes :ASC and skips the rest. This will hide bugs in the user's code as sometimes the order of records happens to be correct by accident.

After the change, this example code won't compile. This means that certain users need to fix their code if this is merged to Korma. But as their code is broken at the moment, I believe it is a good thing. The fix is straightforward:

The example written properly:

(sql/select t
    (sql/order :a :ASC)
    (sql/order :b :ASC)

It might be better, but also more complicated, to alter order to deal with multiple fields properly. That too would break some programs, albeit in a different manner, as broken code would suddenly produce different SQL.

immoh commented 9 years ago

I don't think it would add much value to handle multiple fields properly as the same can be achieved by using order multiple times. I believe it was never meant to support multiple fields, argument destructing was implemented so that it accepted more args but used only the first two.

Thanks!