Merge pull request #2067 from metabase/docstring-checker-as-separate-linter

Make docstring checker separate linter

circle.yml

@@ -26,10 +26,10 @@ test:
     # 1) runs unit tests w/ Postgres local DB. Runs against H2, SQL Server
     # 2) runs unit tests w/ MySQL local DB. Runs against H2, Postgres, SQLite
     # 3) runs unit tests w/ H2 local DB. Runs against H2, Redshift, Druid
-    # 4) runs Eastwood linter & Bikeshed linter && ./bin/reflection-linter
+    # 4) runs docstring-checker linter && Eastwood linter & Bikeshed linter && ./bin/reflection-linter
     # 5) runs JS linter + JS test
     # 6) runs lein uberjar. (We don't run bin/build because we're not really concerned about `npm install` (etc) in this test, which runs elsewhere)
-    - case $CIRCLE_NODE_INDEX in 0) ENGINES=h2,mongo,mysql,bigquery lein test ;; 1) ENGINES=h2,sqlserver MB_DB_TYPE=postgres MB_DB_DBNAME=circle_test MB_DB_PORT=5432 MB_DB_USER=ubuntu MB_DB_HOST=localhost lein test ;; 2) ENGINES=h2,postgres,sqlite MB_DB_TYPE=mysql MB_DB_DBNAME=circle_test MB_DB_PORT=3306 MB_DB_USER=ubuntu MB_DB_HOST=localhost lein test ;; 3) ENGINES=h2,redshift,druid lein test ;; 4) lein eastwood 2>&1 | grep -v Reflection && lein bikeshed 2>&1 | grep -v Reflection && ./bin/reflection-linter ;; 5) npm install && npm run lint && npm run build && npm run test ;; 6) lein uberjar ;; esac:
+    - case $CIRCLE_NODE_INDEX in 0) ENGINES=h2,mongo,mysql,bigquery lein test ;; 1) ENGINES=h2,sqlserver MB_DB_TYPE=postgres MB_DB_DBNAME=circle_test MB_DB_PORT=5432 MB_DB_USER=ubuntu MB_DB_HOST=localhost lein test ;; 2) ENGINES=h2,postgres,sqlite MB_DB_TYPE=mysql MB_DB_DBNAME=circle_test MB_DB_PORT=3306 MB_DB_USER=ubuntu MB_DB_HOST=localhost lein test ;; 3) ENGINES=h2,redshift,druid lein test ;; 4) lein eastwood 2>&1 | grep -v Reflection && lein bikeshed 2>&1 | grep -v Reflection && ./bin/reflection-linter && lein docstring-checker ;; 5) npm install && npm run lint && npm run build && npm run test ;; 6) lein uberjar ;; esac:
         parallel: true
 deployment:
   master:

project.clj

@@ -81,10 +81,15 @@
              :exclude-linters [:constant-test                         ; korma macros generate some forms with if statements that are always logically true or false
                                :suspicious-expression                 ; core.match macros generate some forms like (and expr) which is "suspicious"
                                :unused-ret-vals]}                     ; gives too many false positives for functions with side-effects like conj!
+  :docstring-checker {:include [#"^metabase"]
+                      :exclude [#"test"
+                                #"^metabase\.sample-data$"
+                                #"^metabase\.http-client$"]}
   :profiles {:dev {:dependencies [[org.clojure/tools.nrepl "0.2.12"]  ; REPL <3
                                   [expectations "2.1.3"]              ; unit tests
                                   [ring/ring-mock "0.3.0"]]
-                   :plugins [[jonase/eastwood "0.2.3"
+                   :plugins [[docstring-checker "1.0.0"]              ; Check that all public vars have docstrings
+                             [jonase/eastwood "0.2.3"
                               :exclusions [org.clojure/clojure]]      ; Linting
                              [lein-ancient "0.6.8"                    ; Check project for outdated dependencies + plugins w/ 'lein ancient'
                               :exclusions [org.clojure/clojure]]

src/metabase/task/sync_databases.clj

@@ -9,8 +9,8 @@
                       [task :as task])
             [metabase.models.database :refer [Database]]))
 
-(def sync-databases-job-key "metabase.task.sync-databases.job")
-(def sync-databases-trigger-key "metabase.task.sync-databases.trigger")
+(def ^:private ^:const sync-databases-job-key     "metabase.task.sync-databases.job")
+(def ^:private ^:const sync-databases-trigger-key "metabase.task.sync-databases.trigger")
 
 (defonce ^:private sync-databases-job (atom nil))
 (defonce ^:private sync-databases-trigger (atom nil))

test/metabase/test_setup.clj

 (ns metabase.test-setup
   "Functions that run before + after unit tests (setup DB, start web server, load test data)."
-  (:require (clojure.java [classpath :as classpath]
-                          [io :as io])
+  (:require clojure.data
+            [clojure.java.io :as io]
             [clojure.set :as set]
             [clojure.tools.logging :as log]
-            [clojure.tools.namespace.find :as ns-find]
             [colorize.core :as color]
             [expectations :refer :all]
             (metabase [core :as core]
@@ -97,27 +96,3 @@
   []
   (log/info "Shutting down Metabase unit test runner")
   (core/stop-jetty))
-
-
-
-;; Check that every public var in Metabase has a docstring
-(defn- things-that-need-dox []
-  (sort (for [ns          (ns-find/find-namespaces (classpath/classpath))
-              :let        [nm (try (str (ns-name ns))
-                                   (catch Throwable _))]
-              :when       (and nm
-                               (re-find #"^metabase" nm)
-                               (not (re-find #"test" nm))
-                               (not= nm "metabase.sample-data")
-                               (not= nm "metabase.http-client"))
-              [symb varr] (ns-publics ns)
-              :when       (not (:doc (meta varr)))]
-          (symbol (str (ns-name ns) "/" symb)))))
-
-(defn- throw-if-metabase-doesnt-have-enough-docstrings!
-  {:expectations-options :before-run}
-  []
-  (when-let [things-that-need-dox (seq (things-that-need-dox))]
-    (println (u/format-color 'red "Every public var in Metabase might as well have a docstring! Go write some for the following (or make them ^:private):\n%s"
-               (u/pprint-to-str things-that-need-dox)))
-    (System/exit -1)))