Various small readability and efficiency tweaks #40
Conversation
| (u/strip-nils | ||
| {:table (normalize-reference (.getName t) opts) | ||
| :schema (normalize-reference (.getSchemaName t) opts) | ||
| :instances (when with-instance [t])}))) |
There was a problem hiding this comment.
It's easier to see the shape of the map this way, IMHO.
There was a problem hiding this comment.
Yeah I've debated internally what to do, but we haven't had utils at the time. 😁
| (cond-> (merge a b) | ||
| ;; collect all instances so we can refer back to maps | ||
| (or (-> a :component :instances) | ||
| (-> b :component :instances)) |
There was a problem hiding this comment.
There's no need to check b's instances, consider this truth table:
| a? | b? | union required? | explanation |
|---|---|---|---|
| f | f | f | no instances |
| f | t | f | we already have b's values |
| t | f | t | we have b's component, with no instances |
| t | t | t | we have b's instances only |
|
|
||
| (def ^:private nil-val? (comp nil? val)) | ||
|
|
||
| (defn strip-nils |
There was a problem hiding this comment.
If we wanted to get fancy, we could make this an inline function which, given a map literal, saves us building the intermediate map.
There was a problem hiding this comment.
Given overall performance characteristics I wouldn't worry. :)
| (assoc! acc (key-f k) (val-f v))) | ||
| (transient {}) | ||
| m))] | ||
| (with-meta ret (meta m)))) |
There was a problem hiding this comment.
This matches the semantics of update-keys and update-vals, to avoid later surprises.
| (let [ret (persistent! | ||
| (reduce-kv (fn [acc k v] | ||
| (assoc! acc (key-f k) (val-f v))) | ||
| (transient {}) | ||
| m))] |
There was a problem hiding this comment.
This matches the clojure.core implementations of its sister functions, and saves building a bunch of intermediate vectors.
src/macaw/collect.clj
Outdated
| (u/strip-nils | ||
| {:column (normalize-reference (.getColumnName c) opts) | ||
| :alias (let [[k y] (first ctx)] | ||
| (when (= k :alias) y)) | ||
| :instances (when (:with-instance opts) [c])}))) |
There was a problem hiding this comment.
In this case the difference is even more stark, rather than needing to groc a naming convention, the shape of the map is immediately obvious.
src/macaw/collect.clj
Outdated
| @@ -112,11 +106,12 @@ | |||
|
|
|||
| (defn- make-column [opts ^Column c ctx] | |||
| (merge | |||
| {:column (normalize-reference (.getColumnName c) opts)} | |||
| (maybe-column-alias ctx) | |||
| (maybe-column-table opts c) | |||
There was a problem hiding this comment.
This function is a bit nefarious, as it may also return instances which could clobber our own.
Hmm, perhaps it's worth just pulling out what we want instead of merging it.
There was a problem hiding this comment.
Updated. It's a pity we want multiple keys so we can't get away from the merge.
There was a problem hiding this comment.
Updated again, much happier with destructuring and setting everything explicitly. It's nice not to rely on clobbering.
| (u/strip-nils | ||
| {:table (normalize-reference (.getName t) opts) | ||
| :schema (normalize-reference (.getSchemaName t) opts) | ||
| :instances (when with-instance [t])}))) |
There was a problem hiding this comment.
Yeah I've debated internally what to do, but we haven't had utils at the time. 😁
|
|
||
| (def ^:private nil-val? (comp nil? val)) | ||
|
|
||
| (defn strip-nils |
There was a problem hiding this comment.
Given overall performance characteristics I wouldn't worry. :)
crisptrutski
force-pushed
the
handle-quotes
branch
from
fefb3e1
to
5c3da72
Compare
A few zepto nits that I bit my tongue on in prior reviews.