From b9b8793e3e1c905b1a62064df3c5bcea26fb41c2 Mon Sep 17 00:00:00 2001 From: James Brennan Date: Wed, 6 Apr 2016 21:59:14 -0700 Subject: [PATCH 1/5] Automatically apply suggestions to source files Introduces `--replace` and `--interactive` cli arguments to automatically replace suggestions in source files. Initial port from jpb/kibit-replace b49410c Resolves #155 --- CHANGELOG.md | 1 + README.md | 37 +++++++++++- project.clj | 3 +- src/kibit/driver.clj | 38 ++++++++---- src/kibit/replace.clj | 113 ++++++++++++++++++++++++++++++++++++ test/kibit/test/replace.clj | 29 +++++++++ 6 files changed, 207 insertions(+), 14 deletions(-) create mode 100644 src/kibit/replace.clj create mode 100644 test/kibit/test/replace.clj diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b5f80f..eaef69c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file. This change ## [Unreleased] ### Additions +* Automatic replacement of suggestions (`--replace` and `--interactive` cli arguments) * Enabled Emacs' next error function to go to next Kibit suggestion. See the updated code in the README for the change. diff --git a/README.md b/README.md index 51e83d4..17d6a5b 100644 --- a/README.md +++ b/README.md @@ -44,10 +44,10 @@ If you want to know how the Kibit rule system works there are some slides availa If `lein kibit` returns any suggestions to forms then it's exit code will be 1. Otherwise it will exit 0. This can be useful to add in a build step for automated testing. - $lein kibit + $ lein kibit ... suggestions follow - $echo $? + $ echo $? 1 ## Automatically rerunning when files change @@ -56,7 +56,7 @@ You can use [lein-auto](https://github.com/weavejester/lein-auto) to run kibit a lein-auto's README for installation instructions. Note that this will run kibit over all of your files, not just the ones that have changed. - $lein auto kibit + $ lein auto kibit auto> Files changed: project.clj, [...] auto> Running: lein kibit ... suggestions follow @@ -66,6 +66,37 @@ ones that have changed. ... suggestions follow auto> Failed. +## Automatically replacing suggestions in source file + +You can have kibit automatically apply suggestions to your source files. + +Given a file: + +```clojure +(ns example) + +(+ 1 a) +``` + + $ lein kibit --replace + +will rewrite the file as: + +```clojure +(ns example) + +(+ 1 a) +``` + +Replacement can also be run interactively: + + $ lein kibit --replace --interactive + Would you like to replace + (+ 1 a) + with + (inc a) + in example.clj:3? [yes/no] + ## Reporters Kibit comes with two reporters, the default plaintext reporter, and a GitHub Flavoured Markdown reporter. To specify a reporter, use the `-r` or `--reporter` commandline argument. For example: diff --git a/project.clj b/project.clj index 89d4914..e2d23f2 100644 --- a/project.clj +++ b/project.clj @@ -7,7 +7,8 @@ :comments "Contact if any questions"} :dependencies [[org.clojure/clojure "1.6.0"] [org.clojure/core.logic "0.8.10"] - [org.clojure/tools.cli "0.3.1"]] + [org.clojure/tools.cli "0.3.1"] + [rewrite-clj "0.4.12"]] :profiles {:dev {:dependencies [[lein-marginalia "0.8.0"]] :resource-paths ["test/resources"]}} :deploy-repositories [["releases" :clojars]] diff --git a/src/kibit/driver.clj b/src/kibit/driver.clj index caab14e..b9ef266 100644 --- a/src/kibit/driver.clj +++ b/src/kibit/driver.clj @@ -2,13 +2,20 @@ (:require [clojure.java.io :as io] [kibit.rules :refer [all-rules]] [kibit.check :refer [check-file]] + [kibit.replace :refer [replace-file]] [kibit.reporters :refer :all] [clojure.tools.cli :refer [cli]]) (:import [java.io File])) (def cli-specs [["-r" "--reporter" "The reporter used when rendering suggestions" - :default "text"]]) + :default "text"] + ["-e" "--replace" + "Automatially apply suggestions to source file" + :flag true] + ["-i" "--interactive" + "Interactively prompt before replacing suggestions in source file (Requires `--replace`)" + :flag true]]) (defn ends-with? "Returns true if the java.io.File ends in any of the strings in coll" @@ -31,19 +38,30 @@ (sort-by #(.getAbsolutePath ^File %) (filter clojure-file? (file-seq dir)))) +(defn- run-replace [source-files rules options] + (doseq [file source-files] + (replace-file file + :rules (or rules all-rules) + :interactive (:interactive options)))) + +(defn- run-check [source-files rules {:keys [reporter]}] + (mapcat (fn [file] (try (check-file file + :reporter (name-to-reporter reporter + cli-reporter) + :rules (or rules all-rules)) + (catch Exception e + (binding [*out* *err*] + (println "Check failed -- skipping rest of file") + (println (.getMessage e)))))) + source-files)) + (defn run [source-paths rules & args] (let [[options file-args usage-text] (apply (partial cli args) cli-specs) source-files (mapcat #(-> % io/file find-clojure-sources-in-dir) (if (empty? file-args) source-paths file-args))] - (mapcat (fn [file] (try (check-file file - :reporter (name-to-reporter (:reporter options) - cli-reporter) - :rules (or rules all-rules)) - (catch Exception e - (binding [*out* *err*] - (println "Check failed -- skipping rest of file") - (println (.getMessage e)))))) - source-files))) + (if (:replace options) + (run-replace source-files rules options) + (run-check source-files rules options)))) (defn external-run "Used by lein-kibit to count the results and exit with exit-code 1 if results are found" diff --git a/src/kibit/replace.clj b/src/kibit/replace.clj new file mode 100644 index 0000000..fcdce8a --- /dev/null +++ b/src/kibit/replace.clj @@ -0,0 +1,113 @@ +(ns kibit.replace + (:require [clojure.string :as str] + [clojure.java.io :as io] + [rewrite-clj.zip :as rewrite.zip] + [rewrite-clj.node :as rewrite.node] + [kibit.check :as check] + [kibit.reporters :as reporters])) + +(defn- prompt + "Create a yes/no prompt using the given message. + + From `leiningen.ancient.console`." + [& msg] + (let [msg (str (str/join msg) " [yes/no] ")] + (locking *out* + (loop [i 0] + (when (= (mod i 4) 2) + (println "*** please type in one of 'yes'/'y' or 'no'/'n' ***")) + (print msg) + (flush) + (let [r (or (read-line) "") + r (.toLowerCase ^String r)] + (case r + ("yes" "y") true + ("no" "n") false + (recur (inc i)))))))) + +(defn- report-or-prompt + "" + [file interactive? {:keys [line expr alt]}] + (if interactive? + (prompt (with-out-str + (println "Would you like to replace") + (reporters/pprint-code expr) + (println " with") + (reporters/pprint-code alt) + (print (format "in %s:%s?" file line)))) + (do + (println "Replacing") + (reporters/pprint-code expr) + (println " with") + (reporters/pprint-code alt) + (println (format "in %s:%s" file line)) + + true))) + +(def ^:private expr? (comp not rewrite.node/printable-only? rewrite.zip/node)) + +(defn- map-zipper + "Apply `f` to all code forms in `zipper0`" + [f zipper0] + (let [zipper (if (expr? zipper0) + (rewrite.zip/postwalk zipper0 + expr? + f) + zipper0)] + (if (rewrite.zip/rightmost? zipper) + zipper + (recur f (rewrite.zip/right zipper))))) + +(defn- replace-expr* + "" + [expr reporter kw-opts] + (if-let [check-map (apply check/check-expr + (rewrite.zip/sexpr expr) + kw-opts)] + (if (reporter (assoc check-map + :line + (-> expr rewrite.zip/node meta :row))) + (recur (rewrite.zip/edit expr + (fn -replace-expr [sexpr] + (:alt check-map))) + reporter + kw-opts) + expr) + expr)) + +(defn replace-expr + "Apply any suggestions to `expr`. + + `expr` - Code form to check and replace in + `kw-opts` - any valid option for `check/check-expr`, as well as: + - `:file` current filename + - `:interactive` prompt for confirmation before replacement or not + + Returns a string of the replaced form" + [expr & kw-opts] + (let [options (apply hash-map kw-opts)] + ;; TODO use (:reporter options) to determine format? + (replace-expr* expr + (partial report-or-prompt + (:file options) + (:interactive options)) + kw-opts))) + +(defn replace-file + "Apply any suggestions to `file`. + + `file` - File to check and replace in + `kw-opts` - any valid option for `check/check-expr`, as well as: + - `:interactive` prompt for confirmation before replacement or not + + Modifies `file`, returns `nil`" + [file & kw-opts] + (->> (slurp file) + rewrite.zip/of-string + (map-zipper (fn -replace-expr [node] + (apply replace-expr + node + :file (str file) + kw-opts))) + rewrite.zip/root-string + (spit file))) diff --git a/test/kibit/test/replace.clj b/test/kibit/test/replace.clj new file mode 100644 index 0000000..82d3050 --- /dev/null +++ b/test/kibit/test/replace.clj @@ -0,0 +1,29 @@ +(ns kibit.test.replace + (:require [kibit.check :as check] + [kibit.rules :as rules] + [kibit.replace :as replace]) + (:use [clojure.test]) + (:import java.io.File)) + +(deftest replace-file-are + (are [expected-form test-form] + (= expected-form + (let [file (doto (File/createTempFile "replace-file" ".clj") + (.deleteOnExit) + (spit test-form))] + (with-out-str (replace/replace-file file)) + (slurp file))) + + "(inc a)" + "(+ 1 a)" + + "(ns replace-file) + + (defn \"Documentation\" ^{:my-meta 1} [a] + ;; a comment + (inc a))" + "(ns replace-file) + + (defn \"Documentation\" ^{:my-meta 1} [a] + ;; a comment + (+ 1 a))")) From a09c34e170450f7f20b0117f0cc5d21d995e71a8 Mon Sep 17 00:00:00 2001 From: James Brennan Date: Sun, 24 Apr 2016 15:52:24 -0700 Subject: [PATCH 2/5] Don't have check/check-expr traverse the form replace/check-expr already does a full depth traversal of the form - previously every form was traversed n times (n being the depth of the form). --- src/kibit/replace.clj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/kibit/replace.clj b/src/kibit/replace.clj index fcdce8a..d5b1818 100644 --- a/src/kibit/replace.clj +++ b/src/kibit/replace.clj @@ -63,6 +63,8 @@ [expr reporter kw-opts] (if-let [check-map (apply check/check-expr (rewrite.zip/sexpr expr) + :resolution + :subform kw-opts)] (if (reporter (assoc check-map :line From 08899dbe743032cd70d126a97a2fe97a78393b01 Mon Sep 17 00:00:00 2001 From: James Brennan Date: Sun, 24 Apr 2016 16:27:56 -0700 Subject: [PATCH 3/5] replace-expr should accept a clj form, not a zipper --- src/kibit/replace.clj | 46 +++++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/src/kibit/replace.clj b/src/kibit/replace.clj index d5b1818..5a69b93 100644 --- a/src/kibit/replace.clj +++ b/src/kibit/replace.clj @@ -58,24 +58,35 @@ zipper (recur f (rewrite.zip/right zipper))))) -(defn- replace-expr* +(defn- replace-zipper* "" - [expr reporter kw-opts] + [zipper reporter kw-opts] (if-let [check-map (apply check/check-expr - (rewrite.zip/sexpr expr) + (rewrite.zip/sexpr zipper) :resolution :subform kw-opts)] (if (reporter (assoc check-map :line - (-> expr rewrite.zip/node meta :row))) - (recur (rewrite.zip/edit expr - (fn -replace-expr [sexpr] + (-> zipper rewrite.zip/node meta :row))) + (recur (rewrite.zip/edit zipper + (fn -replace-zipper [sexpr] (:alt check-map))) reporter kw-opts) - expr) - expr)) + zipper) + zipper)) + +(defn- replace-zipper + "" + [zipper & kw-opts] + (let [options (apply hash-map kw-opts)] + ;; TODO use (:reporter options) to determine format? + (replace-zipper* zipper + (partial report-or-prompt + (:file options) + (:interactive options)) + kw-opts))) (defn replace-expr "Apply any suggestions to `expr`. @@ -87,13 +98,14 @@ Returns a string of the replaced form" [expr & kw-opts] - (let [options (apply hash-map kw-opts)] - ;; TODO use (:reporter options) to determine format? - (replace-expr* expr - (partial report-or-prompt - (:file options) - (:interactive options)) - kw-opts))) + (->> (str expr) + rewrite.zip/of-string + (map-zipper (fn -replace-expr [node] + (apply replace-zipper + node + kw-opts))) + rewrite.zip/root + rewrite.node/sexpr)) (defn replace-file "Apply any suggestions to `file`. @@ -106,8 +118,8 @@ [file & kw-opts] (->> (slurp file) rewrite.zip/of-string - (map-zipper (fn -replace-expr [node] - (apply replace-expr + (map-zipper (fn -replace-zipper [node] + (apply replace-zipper node :file (str file) kw-opts))) From 308eae360ce8a768c58969b52b590d7f8435d1cb Mon Sep 17 00:00:00 2001 From: James Brennan Date: Sun, 24 Apr 2016 16:36:30 -0700 Subject: [PATCH 4/5] Remove kibit's location data before replacing --- src/kibit/replace.clj | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/kibit/replace.clj b/src/kibit/replace.clj index 5a69b93..6962d09 100644 --- a/src/kibit/replace.clj +++ b/src/kibit/replace.clj @@ -71,7 +71,11 @@ (-> zipper rewrite.zip/node meta :row))) (recur (rewrite.zip/edit zipper (fn -replace-zipper [sexpr] - (:alt check-map))) + (vary-meta (:alt check-map) + (fn -remove-loc [m] + (dissoc m + :line + :column))))) reporter kw-opts) zipper) From b366ddf1c8a5fd0cef5a04c9b840895e75af631f Mon Sep 17 00:00:00 2001 From: James Brennan Date: Sun, 24 Apr 2016 16:44:09 -0700 Subject: [PATCH 5/5] Add tests for replace-expr --- test/kibit/test/replace.clj | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/test/kibit/test/replace.clj b/test/kibit/test/replace.clj index 82d3050..017f499 100644 --- a/test/kibit/test/replace.clj +++ b/test/kibit/test/replace.clj @@ -3,7 +3,30 @@ [kibit.rules :as rules] [kibit.replace :as replace]) (:use [clojure.test]) - (:import java.io.File)) + (:import java.io.File + java.io.StringWriter)) + +(defmacro discard-output + "Like `with-out-str`, but discards was was written to *out*" + [& body] + `(binding [*out* (StringWriter.)] + ~@body)) + +(deftest replace-expr-are + (are [expected-form test-form] + (= expected-form + (discard-output + (replace/replace-expr test-form))) + + '(inc a) + '(+ 1 a) + + '(defn "Documentation" ^{:my-meta 1} [a] + ;; a comment + (inc a)) + '(defn "Documentation" ^{:my-meta 1} [a] + ;; a comment + (+ 1 a)))) (deftest replace-file-are (are [expected-form test-form] @@ -11,7 +34,7 @@ (let [file (doto (File/createTempFile "replace-file" ".clj") (.deleteOnExit) (spit test-form))] - (with-out-str (replace/replace-file file)) + (discard-output (replace/replace-file file)) (slurp file))) "(inc a)"