Fork me on GitHub
Slack Archive

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

2023-01-13

Channels

#code-reviews
<
2023-01-13
>
mathpunk20:01:47

Here is my dumb little function for paring down a given data shape: 

(defn failures [{:keys [results]}]
  (if-not (= 1 (count results))
    (throw "Expectations failed: There was more than one result")
    (let [suites (-> (first results) (get-in [:suites]))]
      (if-not (= 1 (count suites))
        (throw "Expectations failed: There was more than one suite")
        (->> (-> suites first (get :tests))
             (filter :fail))))))
By inspection, I’ve observed that it looks like the results and suites are always a sequence containing one map, rather than a map. I dunno why, it’s just what gets spit out. But what if I’m wrong? I’ll want to know, and update my assumptions. I feel like this if-not… throw pattern is probably better handled by spec, right? When you’re trying to be quick and dirty and use an observation, but you know it might be wrong in the future and you want to do future self a favor, how would you handle this?

moe05:01:53

If prefer when for throwing exceptions. You could use function pre-conditions for these guys if you didn't want to go full spec

💡 2
moe05:01:55

{:pre [(= 1 (count results)) (-> results first :suites count (= 1))]}

thanks2 2
moe05:01:22

It's a personal thing, but I never use the keys destructuring form. I think it looks incongruous. Even if I'm mapping keywords to vars in the same way, I'll just spell it out

Eric Scott23:01:17

Containers which I only expect to have one member happens often enough that I keep a special utility function. 

(defn unique
  "Returns the single member of `coll`, or nil if `coll` is empty. Calls `on-ambiguity` if there is more than one member (default is to throw an Exception).
  Where
  -   `coll` is a collection
  -   `on-ambiguity` := (fn [coll] ...) -> `value`, default raises an error.
 
  "
  ([coll on-ambiguity]
   (if (seq coll)
     (if (> (count coll) 1)
       (on-ambiguity coll)
       (first coll))))
  ([coll]
   (unique coll (fn [coll]
                  (throw (ex-info "Unique called on non-unique collection"
                                  {:type ::NonUnique
                                   :coll coll
                                   }))))))
Using that, you could re-write your code as something like: 
(defn failures [m]
  (let [suite (-> m
                 (:results)
                 (unique)
                 (:suites)
                 (unique)
         ]
   (filter :fail (:tests suite)))

2
Eric Scott23:01:36

Having looked at this code for the first time in a while, I think the preferred order for those two versions of unique should be in the opposite order with the shorter arg list first. Also, I should probably` (assert (coll seq))` and not make it a conditional.

📝 2
mathpunk00:01:19

thanks y'all!

❤️ 2
jaihindhreddy11:01:58

I find this funny: 

(defn failures
  [{[{[{:keys [tests]}
       :as suites] :suites}
     :as results] :results}]
  {:pre [(= 1 (count results))
         (= 1 (count suites))]}
  (filter :fail tests))

moe11:01:15

:keys is truly an abomination

moe11:01:39

but so is that indentation

2
moe11:01:32

(defn failures {[{[{tests :tests} & suites] :suites} & results] :results}]
  {:pre [(not suites) (not results)]}
  (filter :fail tests))

jaihindhreddy11:01:55

yeah, just single line looks much better. Still. That's quite a bit going-on in the arglist. Also, the two & should be :as AFAICT.

moe11:01:25

No, it's taking the tail of the vectors to make the precondition simpler

💯 2
jaihindhreddy11:01:35

oh yeah, didn't read the changed pre-conditions. Nice!

moe12:01:48

but yeah, that's not a winning function signature