LauJensen / clojureql

ClojureQL is superior SQL integration for Clojure
https://clojureql.sabrecms.com
Eclipse Public License 1.0
285 stars 39 forks source link

not-lilke via defoperator #28

Closed ck closed 13 years ago

ck commented 13 years ago

This is probably not idiomatic Clojure code, esp. since it invokes a method on the Java String class, but it seemed the 'easiest' solution to me. I played around with clojure.string/split and clojure.string/replace, but both of them fell to heavy to me.

As mentioned in commit message, in the solution the parameterize leaves only the first keyword, e.g. :not-like ~> " :not like ". Although it works, my gut tells me this is not good code.

Any feedback is appreciated,

-ck

PS: Appreciate the patience.

LauJensen commented 13 years ago

Hi Ck,

Here are my initial thoughts.

1) The result of running not-like is (y :not like ?), why is there a keyword in there? (:not), it shouldnt be. 2) Most places in CQL we used strings for non-standard stuff. Did you consider supporting

(defoperator not-like "NOT LIKE" .....) ?

I think that might be the most 'idiomatic' way to go, and it only requires the 'if' statement in parameterize to be replaced with a cond, which checks for the type of its argument.

Let me know what you think Lau

ck commented 13 years ago

Lau,

regarding 1) I agree with you and I it did not seem correct to me either, but playing around with the code to understand the call stack, I found that it leaves the symbol in there as well

user=> (use 'clojureql.predicates)     
nil
user=> (parameterize :like '(:x "foo%"))
"(x :like ?)"

Let me know if that was an unintended effect and can be changed. I will take a look at supporting strings as well.

Thank you for your patience,

-ck

LauJensen commented 13 years ago

It can and should be changed. The interfaces should be

"NOT LIKE" => "NOT LIKE" :like => "LIKE" :not-like => "NOT-LIKE" <-- Shouldnt be used by the user

So, fixing the 'if' in paramterize should be the key. You're also very welcome to ammend /test/clojureql/test/predicate.clj once this is working.

Thanks! Lau

ck commented 13 years ago

Makes sense. I do not think the if statement in the paramterize needed to be changed, since it does not touch the op. Please take a look at

https://github.com/ck/clojureql/commit/994ea3a4406b340d057b4df5d8b8952ff4e50780

It looks clean to me. The only slight disturbance is a call to name independent of the argument type (keyword or string). But an if statement would occur the same cost and result in more code chars/lines.

Again, any feedback is greatly appreciated,

-ck

LauJensen commented 13 years ago

You're right of course, I had misread my own function, sorry about the confusion!

Your latest patch is great, so Ive pulled it, trimmed it and merged it.

What I did was add a helper called upper-name, which is like upper-case but it also calls name. This makes :like become LIKE and "not liKe" become "NOT LIKE". Then I removed your foo/bar references in the test file. Generally I try to avoid those two placeholders and use something more descriptive - I realize however, that Im the odd-ball in this regard :) And finally I aligned the columns in the test file for easier reading.

Thanks a lot for pitching in, hope to see more patches from you in the future, Lau