Implement code review suggestions

src/metabase/automagic_dashboards/comparison.clj

@@ -68,7 +68,7 @@
                                          :visualization_settings {}
                                          :id                     (gensym)}))))))
 
-(def ^:private ^Long title-height 2)
+(def ^:private ^Long ^:const title-height 2)
 
 (defn- add-col-title
   [dashboard title col]

src/metabase/automagic_dashboards/core.clj

@@ -492,7 +492,7 @@
                     :table       table})))
          (sort-by :score >))))
 
-(def ^:private ^Long max-related 6)
+(def ^:private ^Long ^:const max-related 6)
 
 (defn automagic-dashboard
   "Create dashboards for table `root` using the best matching heuristics."

src/metabase/automagic_dashboards/populate.clj

@@ -7,13 +7,13 @@
             [metabase.models.card :as card]
             [toucan.db :as db]))
 
-(def ^Long grid-width
+(def ^Long ^:const grid-width
   "Total grid width."
   18)
-(def ^Long default-card-width
+(def ^Long ^:const default-card-width
   "Default card width."
   6)
-(def ^Long default-card-height
+(def ^Long ^:const default-card-height
   "Default card height"
   4)
 
@@ -74,7 +74,7 @@
            :card                   nil
            :id                     (gensym)}))
 
-(def ^:private ^Long max-cards 9)
+(def ^:private ^Long ^:const max-cards 9)
 
 (defn- make-grid
   [width height]
@@ -131,7 +131,7 @@
 (def ^:private ^{:arglists '([card])} text-card?
   :text)
 
-(def ^:private ^Long group-heading-height 2)
+(def ^:private ^Long ^:const group-heading-height 2)
 
 (defn- add-group
   [dashboard grid group cards]

src/metabase/automagic_dashboards/rules.clj

@@ -11,7 +11,7 @@
              [core :as s]]
             [yaml.core :as yaml]))
 
-(def ^Long max-score
+(def ^Long ^:const max-score
   "Maximal (and default) value for heuristics scores."
   100)
 

src/metabase/sync/analyze/classifiers/name.clj

@@ -139,6 +139,7 @@
    [#"vendor"       :entity/CompanyTable]])
 
 (s/defn ^:always-validate infer-entity-type :- i/TableInstance
+  "Classifer that infers the special type of a TABLE based on its name."
   [table :- i/TableInstance]
   (let [table-name (-> table :name str/lower-case)]
     (assoc table :entity_type (or (some (fn [[pattern type]]

src/metabase/sync/analyze/classify.clj

@@ -44,7 +44,7 @@
 (def ^:private FieldOrTableInstance (s/either i/FieldInstance i/TableInstance))
 
 (s/defn ^:private save-model-updates!
-  "Save the updates in UPDATED-FIELD."
+  "Save the updates in `updated-model` (can be either a `Field` or `Table`)."
   [original-model :- FieldOrTableInstance, updated-model :- FieldOrTableInstance]
   (assert (= (type original-model) (type updated-model)))
   (let [[_ values-to-set] (data/diff original-model updated-model)]
@@ -55,7 +55,7 @@
     (doseq [k (keys values-to-set)]
       (when-not (contains? values-that-can-be-set k)
         (throw (Exception. (format "Classifiers are not allowed to set the value of %s." k)))))
-    ;; cool, now we should be ok to update the Field
+    ;; cool, now we should be ok to update the model
     (when values-to-set
       (db/update! (if (instance? (type Field) original-model)
                     Field

test/metabase/automagic_dashboards/core_test.clj

@@ -19,22 +19,31 @@
                                                     test-users/user->id
                                                     user/permissions-set
                                                     atom)]
-    ~@body))
+     ~@body))
+
 
 (expect
-  [[:field-id 1]
-   [:fk-> 1 2]
-   42]
-  (map (partial #'magic/->reference :mbql)
-       [(-> (field/->FieldInstance) (assoc :id 1))
-        (-> (field/->FieldInstance) (assoc :id 1 :fk_target_field_id 2))
-        42]))
+  [:field-id 1]
+  (->> (assoc (field/->FieldInstance) :id 1)
+       (#'magic/->reference :mbql)))
 
 (expect
-  [[:entity/UserTable :entity/GenericTable :entity/*]]
-  (map (comp (partial map :table_type)
-             (partial #'magic/matching-rules (rules/load-rules)))
-       [(table/map->TableInstance {:entity_type :entity/UserTable})]))
+  [:fk-> 1 2]
+  (->> (assoc (field/->FieldInstance) :id 1 :fk_target_field_id 2)
+       (#'magic/->reference :mbql)))
+
+(expect
+  42
+  (->> 42
+       (#'magic/->reference :mbql)))
+
+
+(expect
+  [:entity/UserTable :entity/GenericTable :entity/*]
+  (->> (table/map->TableInstance {:entity_type :entity/UserTable})
+       (#'magic/matching-rules (rules/load-rules))
+       (map :table_type)))
+
 
 (expect
   true
@@ -42,27 +51,62 @@
     (tu/with-model-cleanup ['Card 'Dashboard 'Collection 'DashboardCard]
       (-> (keep automagic-dashboard (Table)) count pos?))))
 
+
+;; Identity
+(expect
+  :d1
+  (-> [{:d1 {:field_type [:type/Category] :score 100}}]
+      (#'magic/most-specific-definition)
+      first
+      key))
+
+;; Identity
+(expect
+  :d1
+  (-> [{:d1 {:field_type [:type/Category] :score 100}}]
+      (#'magic/most-specific-definition)
+      first
+      key))
+
+;; Base case: more ancestors
+(expect
+  :d2
+  (-> [{:d1 {:field_type [:type/Category] :score 100}}
+       {:d2 {:field_type [:type/State] :score 100}}]
+      (#'magic/most-specific-definition)
+      first
+      key))
+
+;; Break ties based on the number of additional filters
+(expect
+  :d3
+  (-> [{:d1 {:field_type [:type/Category] :score 100}}
+       {:d2 {:field_type [:type/State] :score 100}}
+       {:d3 {:field_type [:type/State]
+             :named      "foo"
+             :score      100}}]
+      (#'magic/most-specific-definition)
+      first
+      key))
+
+;; Break ties on score
+(expect
+  :d2
+  (-> [{:d1 {:field_type [:type/Category] :score 100}}
+       {:d2 {:field_type [:type/State] :score 100}}
+       {:d3 {:field_type [:type/State] :score 90}}]
+      (#'magic/most-specific-definition)
+      first
+      key))
+
+;; Number of additional filters has precedence over score
 (expect
-  [:d1 :d2 :d3 :d2 :d3]
-  (map (comp key first #'magic/most-specific-definition)
-       [; Identity
-        [{:d1 {:field_type [:type/Category] :score 100}}]
-        ; Base case: more ancestors
-        [{:d1 {:field_type [:type/Category] :score 100}}
-         {:d2 {:field_type [:type/State] :score 100}}]
-        ; Break ties based on the number of additional filters
-        [{:d1 {:field_type [:type/Category] :score 100}}
-         {:d2 {:field_type [:type/State] :score 100}}
-         {:d3 {:field_type [:type/State]
-               :named      "foo"
-               :score      100}}]
-        ; Break ties on score
-        [{:d1 {:field_type [:type/Category] :score 100}}
-         {:d2 {:field_type [:type/State] :score 100}}
-         {:d3 {:field_type [:type/State] :score 90}}]
-        ; Number of additional filters has precedence over score
-        [{:d1 {:field_type [:type/Category] :score 100}}
-         {:d2 {:field_type [:type/State] :score 100}}
-         {:d3 {:field_type [:type/State]
-               :named      "foo"
-               :score      0}}]]))
+  :d3
+  (-> [{:d1 {:field_type [:type/Category] :score 100}}
+       {:d2 {:field_type [:type/State] :score 100}}
+       {:d3 {:field_type [:type/State]
+             :named      "foo"
+             :score      0}}]
+      (#'magic/most-specific-definition)
+      first
+      key))

test/metabase/automagic_dashboards/rules_test.clj

@@ -2,39 +2,25 @@
   (:require [expectations :refer :all]
             [metabase.automagic-dashboards.rules :refer :all :as rules]))
 
-(expect
-  [nil
-   [nil]
-   [42]
-   [42]]
-  (map #'rules/ensure-seq [nil [nil] 42 [42]]))
+(expect nil  (#'rules/ensure-seq nil))
+(expect nil  (#'rules/ensure-seq [nil]))
+(expect [42] (#'rules/ensure-seq 42))
+(expect [42] (#'rules/ensure-seq [42]))
 
-(expect
-  [true
-   false]
-  (map ga-dimension? ["ga:foo" "foo"]))
+(expect true  (ga-dimension? "ga:foo"))
+(expect false (ga-dimension? "foo"))
 
-(expect
-  [:foo
-   "ga:foo"
-   :type/Foo]
-  (map #'rules/->type [:foo "ga:foo" "Foo"]))
+(expect :foo      (#'rules/->type :foo))
+(expect "ga:foo"  (#'rules/->type "ga:foo"))
+(expect :type/Foo (#'rules/->type "Foo"))
 
-(expect
-  true
-  (every? some? (load-rules)))
+(expect true (every? some? (load-rules)))
 
-(expect
-  [true
-   true
-   true
-   false
-   false]
-  (map (comp boolean #'rules/dimension-form?) [[:dimension "Foo"]
-                                               ["dimension" "Foo"]
-                                               ["DIMENSION" "Foo"]
-                                               42
-                                               [:baz :bar]]))
+(expect true  (dimension-form? [:dimension "Foo"]))
+(expect true  (dimension-form? ["dimension" "Foo"]))
+(expect true  (dimension-form? ["DIMENSION" "Foo"]))
+(expect false (dimension-form? 42))
+(expect false (dimension-form? [:baz :bar]))
 
 (expect
   ["Foo" "Baz" "Bar"]

test/metabase/util_test.clj

@@ -243,18 +243,28 @@
     :present #{:a :b :c}
     :non-nil #{:d :e :f}))
 
-(expect
-  [-2 -1 0 1 2 3 0 3]
-  (map order-of-magnitude [0.01 0.5 4 12 444 1023 0 -1444]))
 
-(expect
-  [{:foo 2}
-   {:foo 2 :bar 3}]
-  [(update-when {:foo 2} :bar inc)
-   (update-when {:foo 2 :bar 2} :bar inc)])
+;;; tests for `order-of-magnitude`
+(expect -2 (order-of-magnitude 0.01))
+(expect -1 (order-of-magnitude 0.5))
+(expect 0  (order-of-magnitude 4))
+(expect 1  (order-of-magnitude 12))
+(expect 2  (order-of-magnitude 444))
+(expect 3  (order-of-magnitude 1023))
+(expect 0  (order-of-magnitude 0))
+(expect 3  (order-of-magnitude -1444))
 
-(expect
-  [{:foo 2}
-   {:foo {:bar 3}}]
-  [(update-in-when {:foo 2} [:foo :bar] inc)
-   (update-in-when {:foo {:bar 2}} [:foo :bar] inc)])
+
+;;; tests for `update-when` and `update-in-when`
+(expect {:foo 2}        (update-when {:foo 2} :bar inc))
+(expect {:foo 2 :bar 3} (update-when {:foo 2 :bar 2} :bar inc))
+
+(expect {:foo 2}        (update-in-when {:foo 2} [:foo :bar] inc))
+(expect {:foo {:bar 3}} (update-in-when {:foo {:bar 2}} [:foo :bar] inc))
+
+
+;;; tests for `index-of`
+(expect 2   (index-of pos? [-1 0 2 3]))
+(expect nil (index-of pos? [-1 0 -2 -3]))
+(expect nil (index-of pos? nil))
+(expect nil (index-of pos? []))