Augment a given project's :acceptable-aliases-whitelist with the aliases it already uses (#205)

Author: vemv

project.clj

@@ -128,7 +128,7 @@
                                      :resource-paths ["test-resources-extra"
                                                       "test-resources"]}
 
-             :refactor-nrepl        {:dependencies [[refactor-nrepl "3.3.2"]
+             :refactor-nrepl        {:dependencies [[refactor-nrepl "3.5.2"]
                                                     [nrepl "0.9.0"]]
                                      ;; cider-nrepl is a :provided dependency from refactor-nrepl.
                                      :plugins      [[cider/cider-nrepl "0.28.2" :exclusions [nrepl]]]}

src/formatting_stack/branch_formatter.clj

@@ -82,7 +82,7 @@
   (pretty-printer/new {}))
 
 (defn format-and-lint-branch! [& {:keys [target-branch in-background? reporter]
-                                  :or   {target-branch  "master"
+                                  :or   {target-branch  (strategies/default-branch-name)
                                          in-background? (not (System/getenv "CI"))
                                          reporter       default-reporter}}]
   (let [default-strategies [(fn [& {:as options}]
@@ -97,7 +97,7 @@
                                    :in-background? in-background?)))
 
 (defn lint-branch! [& {:keys [target-branch in-background? reporter]
-                       :or   {target-branch  "master"
+                       :or   {target-branch  (strategies/default-branch-name)
                               in-background? false
                               reporter       default-reporter}}]
   (let [default-strategies [(fn [& {:as options}]

src/formatting_stack/linters/ns_aliases.clj

@@ -2,8 +2,10 @@
   "Observes these guidelines: https://stuartsierra.com/2015/05/10/clojure-namespace-aliases"
   (:require
    [clojure.string :as string]
+   [formatting-stack.linters.ns-aliases.impl :as impl]
    [formatting-stack.protocols.linter :as linter]
-   [formatting-stack.util :refer [ensure-coll ensure-sequential process-in-parallel!]]
+   [formatting-stack.strategies :as strategies]
+   [formatting-stack.util :refer [ensure-sequential process-in-parallel!]]
    [nedap.utils.modular.api :refer [implement]]))
 
 (defn clause= [a b]
@@ -88,8 +90,17 @@
                                              :source              :formatting-stack/ns-aliases})))))
        (mapcat ensure-sequential)))
 
-(defn new [{:keys [acceptable-aliases-whitelist]
-            :or   {acceptable-aliases-whitelist default-acceptable-aliases-whitelist}}]
+(defn new
+  "If `:augment-acceptable-aliases-whitelist?` is true,
+  all aliases already used in your current project (as Git status and branch info indicates) will be deemed acceptable."
+  [{:keys [acceptable-aliases-whitelist
+           augment-acceptable-aliases-whitelist?]
+    :or   {acceptable-aliases-whitelist default-acceptable-aliases-whitelist
+           augment-acceptable-aliases-whitelist? true}}]
   (implement {:id ::id
-              :acceptable-aliases-whitelist acceptable-aliases-whitelist}
+              :acceptable-aliases-whitelist
+              (cond-> acceptable-aliases-whitelist
+                (and augment-acceptable-aliases-whitelist?
+                     impl/namespace-aliases-for*)
+                (impl/merge-aliases (impl/project-aliases {:cache-key (strategies/current-branch-name)})))}
     linter/--lint! lint!))

src/formatting_stack/linters/ns_aliases/impl.clj

@@ -0,0 +1,55 @@
+(ns formatting-stack.linters.ns-aliases.impl
+  (:require
+   [clojure.set :as set]
+   [clojure.spec.alpha :as spec]
+   [formatting-stack.strategies :as strategies]
+   [nedap.speced.def :as speced])
+  (:import
+   (java.io File)))
+
+(spec/def ::alias (spec/or :simple-alias symbol?
+                           :directive keyword?))
+
+(spec/def ::aliases (spec/coll-of ::alias :kind vector?))
+
+(spec/def ::project-aliases (spec/map-of symbol? ::aliases))
+
+(speced/defn ^::project-aliases merge-aliases [^::project-aliases m1, ^::project-aliases m2]
+  (merge-with (fn [x y]
+                (vec (into #{} cat [x y])))
+              m1
+              m2))
+
+(def namespace-aliases-for*
+  (when (strategies/refactor-nrepl-3-4-1-available?)
+    @(requiring-resolve 'refactor-nrepl.ns.libspecs/namespace-aliases-for)))
+
+(defn namespace-aliases-for [files]
+  (when namespace-aliases-for*
+    (let [{:keys [clj cljs]} (namespace-aliases-for* files true)]
+      (merge-aliases clj cljs))))
+
+;; NOTE: this isn't necessarily a "strategy" (which would reside in the `strategies` ns),
+;; since it's composed of other strategy calls.
+;; This is more of a handy helper at the moment.
+(defn stable-files
+  "Files that already existed as of the default branch,
+  and that haven't been touched in the current branch"
+  []
+  (let [with (set (strategies/all-files :files []))
+        without (into #{} cat [(strategies/git-diff-against-default-branch :files [])
+                               (strategies/git-completely-staged :files [])
+                               (strategies/git-not-completely-staged :files [])])
+        corpus (set/difference with without)]
+    (->> corpus
+         (mapv (speced/fn [^String s]
+                 (File. s))))))
+
+(def project-aliases
+  (memoize
+   (fn [{_cache-key :cache-key}] ;; there's a cache key for correct memoization
+
+     ;; note that memoizing results is correct -
+     ;; results don't have to be recomputed as the git status changes:
+     ;; touching more files doesn't alter the fact that these aliases already were existing.
+     (namespace-aliases-for (stable-files)))))

src/formatting_stack/processors/test_runner.clj

@@ -8,7 +8,7 @@
    [clojure.test]
    [formatting-stack.processors.test-runner.impl :refer [ns->sym testable-namespaces]]
    [formatting-stack.protocols.processor :as processor]
-   [formatting-stack.strategies :refer [git-completely-staged git-diff-against-default-branch git-not-completely-staged]]
+   [formatting-stack.strategies :as strategies :refer [git-completely-staged git-diff-against-default-branch git-not-completely-staged]]
    [nedap.utils.modular.api :refer [implement]]))
 
 ;; Not provided into any default stack, as it would be overly assuming about users' practices
@@ -32,7 +32,7 @@
   Out of those files, namespaces are derived (1:N, using smart heuristics),
   and those namespaces are run via `#'clojure.test/run-tests`."
   [& {:keys [target-branch]
-      :or   {target-branch "master"}}]
+      :or   {target-branch (strategies/default-branch-name)}}]
   (let [filenames (->> (git-diff-against-default-branch :target-branch target-branch)
                        (concat (git-completely-staged :files []))
                        (concat (git-not-completely-staged :files []))

src/formatting_stack/project_formatter.clj

@@ -59,7 +59,9 @@
    (-> (linters.one-resource-per-ns/new {})
        (assoc :strategies (conj default-strategies
                                 strategies/files-with-a-namespace)))
-   (-> (linters.ns-aliases/new {})
+   (-> (linters.ns-aliases/new {;; When linting an entire project, this option should be false
+                                ;; since the user's intent is clearly to not particularly regard the current formatting:
+                                :augment-acceptable-aliases-whitelist? false})
        (assoc :strategies (conj default-strategies
                                 strategies/files-with-a-namespace
                                 ;; reader conditionals may confuse `linters.ns-aliases`

src/formatting_stack/strategies.clj

@@ -12,6 +12,7 @@
 
   A strategy may not return nil."
   (:require
+   [clojure.java.shell :refer [sh]]
    [clojure.string :as string]
    [clojure.tools.namespace.repl :as tools.namespace.repl]
    [formatting-stack.protocols.spec :as protocols.spec]
@@ -27,7 +28,7 @@
 (def git-command "git")
 
 (speced/defn all-files
-  "This strategy unconditionally processes all files."
+  "This strategy unconditionally processes all Clojure and ClojureScript files."
   [& {:keys [^::protocols.spec/filenames files]}]
   ;; This first `binding` is necessary for obtaining an absolutized list of deletions
   (binding [impl/*skip-existing-files-check?* true]
@@ -80,11 +81,34 @@
        (impl/extract-clj-files)
        (into files)))
 
+(defn current-branch-name []
+  (-> (sh "git" "rev-parse" "--abbrev-ref" "HEAD")
+      (:out)
+      (string/split-lines)
+      (first)))
+
+(defn default-branch-name []
+  (let [fallback-property-name "formatting-stack.default-branch-name"
+        fallback-branch-name "master"]
+    (or (not-empty (System/getProperty fallback-property-name))
+        (let [all-branches (->> (sh "git" "branch")
+                                :out
+                                string/split-lines
+                                (map (fn [s]
+                                       (-> s (string/split #"\s+") last)))
+                                (set))]
+          (or (some all-branches ["master" "main" "stable" "dev"])
+              (do
+                (println (format "No default branch could be determined. Falling back to `%s`.
+You can choose another one by setting the `%s` system property." fallback-branch-name fallback-property-name))
+                ;; return something, for not breaking code that traditionally assumed "master":
+                fallback-branch-name))))))
+
 (defn git-diff-against-default-branch
   "This strategy processes all files that this branch has modified.
   The diff is compared against the `:target-branch` option."
   [& {:keys [target-branch impl files blacklist]
-      :or   {target-branch "master"
+      :or   {target-branch (default-branch-name)
              ;; We filter for Added, Copied, Modified and Renamed files,
              ;; excluding Unmerged, Deleted, Type-changed, Broken (pair), and Unknown files
              impl          (impl/file-entries git-command "diff" "--name-only" "--diff-filter=ACMR" target-branch "--")
@@ -193,6 +217,14 @@
       (catch Throwable _
         false))))
 
+(defn refactor-nrepl-3-4-1-available? []
+  (locking require-lock
+    (try
+      (requiring-resolve 'refactor-nrepl.ns.libspecs/namespace-aliases-for)
+      true
+      (catch Throwable _
+        false))))
+
 (speced/defn when-refactor-nrepl
   "This strategy leaves all `files` as-is iff the `refactor-nrepl` library is in the classpath;
   else all `files` will be filtered out."

test/functional/formatting_stack/linters/ns_aliases.clj

@@ -1,12 +1,15 @@
 (ns functional.formatting-stack.linters.ns-aliases
   (:require
-   [clojure.test :refer [are deftest]]
+   [clojure.test :refer [are deftest testing]]
    [formatting-stack.linters.ns-aliases :as sut]
+   [formatting-stack.linters.ns-aliases.impl :as sut.impl]
    [formatting-stack.protocols.linter :as linter]
+   [formatting-stack.strategies :as strategies]
    [matcher-combinators.test :refer [match?]]))
 
 (deftest lint!
-  (let [linter (sut/new {:max-lines-per-ns 4})]
+  (let [linter (sut/new {:max-lines-per-ns 4
+                         :augment-acceptable-aliases-whitelist? false})]
     (are [filename expected] (match? expected
                                      (linter/lint! linter [filename]))
       "test-resources/valid_syntax.clj"
@@ -21,4 +24,17 @@
         :column              4
         :warning-details-url "https://stuartsierra.com/2015/05/10/clojure-namespace-aliases"
         :msg                 "[clojure.string :as foo] is not a derived alias."
-        :filename            "test-resources/ns_aliases_warning.clj"}])))
+        :filename            "test-resources/ns_aliases_warning.clj"}]))
+
+  (when (strategies/refactor-nrepl-3-4-1-available?)
+    (testing "`:augment-acceptable-aliases-whitelist?`"
+      (are [input expected] (match? expected
+                                    (linter/lint! (sut/new {:augment-acceptable-aliases-whitelist? input})
+                                                  ["test-resources/ns_aliases_warning.clj"]))
+        true  []
+        false [{:source              :formatting-stack/ns-aliases
+                :line                3
+                :column              4
+                :warning-details-url "https://stuartsierra.com/2015/05/10/clojure-namespace-aliases"
+                :msg                 "[clojure.string :as foo] is not a derived alias."
+                :filename            "test-resources/ns_aliases_warning.clj"}]))))

test/integration/formatting_stack/strategies.clj

@@ -167,14 +167,14 @@
     ;; Creating a file which collides with a ref, should not impact #'sut/git-diff-against-default-branch
     ;; see https://git.io/JKG8m
     (let [git-sha-named-file (io/file git-integration-dir @root-commit)]
-     (try
+      (try
        ;; creating a file with a filename which collides with a sha.
-       (spit git-sha-named-file creatable-contents)
-       (expect-sane-output! (sut/git-diff-against-default-branch :target-branch @root-commit))
-       (finally
-         (sh "git" "reset" "--" @root-commit)
-         (-> git-sha-named-file .delete)
-         (cleanup-testing-repo!))))))
+        (spit git-sha-named-file creatable-contents)
+        (expect-sane-output! (sut/git-diff-against-default-branch :target-branch @root-commit))
+        (finally
+          (sh "git" "reset" "--" @root-commit)
+          (-> git-sha-named-file .delete)
+          (cleanup-testing-repo!))))))
 
 (deftest git-not-completely-staged
 

test/integration/formatting_stack/strategies/default_branch_name.clj

@@ -0,0 +1,8 @@
+(ns integration.formatting-stack.strategies.default-branch-name
+  (:require
+   [clojure.test :refer [deftest is]]
+   [formatting-stack.strategies :as sut]))
+
+;; This deftest lives in its own ns, for not having unrelated fixtures interfere
+(deftest default-branch-name
+  (is (= "main" (sut/default-branch-name))))