Fixes TCLI-98 by adding :post-validation flag

Author: seancorfield

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Change Log
 
 * Release 1.0.next (on **master**)
+  * Allow validation to be performed either after parsing (before option processing) -- current default -- or after option processing, via the `:post-validation true` flag [TCLI-98](https://clojure.atlassian.net/browse/TCLI-98).
   * Allow validation message to be a function (of the invalid argument) in addition to being a plain string [TCLI-97](https://clojure.atlassian.net/browse/TCLI-97).
   * Add `:multi true` to modify behavior of `:update-fn` [TCLI-96](https://clojure.atlassian.net/browse/TCLI-96).
 

README.md

@@ -172,6 +172,16 @@ be useful within your own `:summary-fn` for generating the default summary.
 
 ### Option Argument Validation
 
+By default, option validation is performed immediately after parsing, which
+means that "flag" arguments will have a Boolean value, even if a `:default`
+is specified with a different type of value.
+
+You can choose to perform validation after option processing instead, with
+the `:post-validation true` flag. During option processing, `:default` values
+are applied and `:assoc-fn` and `:update-fn` are invoked. If an option is
+specified more than once, `:post-validation true` will cause validation to
+be performed after each new option value is processed.
+
 There is a new option entry `:validate`, which takes a tuple of
 `[validation-fn validation-msg]`. The validation-fn receives an option's
 argument *after* being parsed by `:parse-fn` if it exists. The validation-msg

src/main/clojure/clojure/tools/cli.cljc

@@ -237,7 +237,8 @@
 
 (def ^{:private true} spec-keys
   [:id :short-opt :long-opt :required :desc :default :default-desc :default-fn
-   :parse-fn :assoc-fn :update-fn :multi :validate-fn :validate-msg :missing])
+   :parse-fn :assoc-fn :update-fn :multi :post-validation
+   :validate-fn :validate-msg :missing])
 
 (defn- select-spec-keys
   "Select only known spec entries from map and warn the user about unknown
@@ -306,6 +307,7 @@
    :validate-msg [String] ; [\"Must be an IPv4 host\"
                           ;  \"Must not be a multicast address\"]
                           ; can also be a function (of the invalid argument)
+   :post-validation Boolean ; default false
    :missing      String   ; \"server must be specified\"
    }
 
@@ -382,17 +384,17 @@
 (defn- parse-error [opt optarg msg]
   (str "Error while parsing option " (pr-join opt optarg) ": " msg))
 
-(defn- validation-error [opt optarg msg]
+(defn- validation-error [value opt optarg msg]
   (str "Failed to validate " (pr-join opt optarg)
-       (if msg (str ": " (if (string? msg) msg (msg optarg))) "")))
+       (if msg (str ": " (if (string? msg) msg (msg value))) "")))
 
 (defn- validate [value spec opt optarg]
   (let [{:keys [validate-fn validate-msg]} spec]
     (or (loop [[vfn & vfns] validate-fn [msg & msgs] validate-msg]
           (when vfn
             (if (try (vfn value) (catch #?(:clj Throwable :cljs :default) _))
               (recur vfns msgs)
-              [::error (validation-error opt optarg msg)])))
+              [::error (validation-error value opt optarg msg)])))
         [value nil])))
 
 (defn- parse-value [value spec opt optarg]
@@ -403,9 +405,12 @@
                           (catch #?(:clj Throwable :cljs :default) e
                             [nil (parse-error opt optarg (str e))]))
                         [value nil])]
-    (if error
-      [::error error]
-      (validate value spec opt optarg))))
+    (cond error
+          [::error error]
+          (:post-validation spec)
+          [value nil]
+          :else
+          (validate value spec opt optarg))))
 
 (defn- neg-flag? [spec opt]
   (and (:long-opt spec)
@@ -452,13 +457,17 @@
                            (or (find-spec specs :short-opt optarg)
                                (find-spec specs :long-opt optarg)))
                     [m ids (conj errors (missing-required-error opt (:required spec)))]
-                    [(if-let [update-fn (:update-fn spec)]
-                       (if (:multi spec)
-                         (update m id update-fn value)
-                         (update m id update-fn))
-                       ((:assoc-fn spec assoc) m id value))
-                     (conj ids id)
-                     errors])
+                    (let [m' (if-let [update-fn (:update-fn spec)]
+                               (if (:multi spec)
+                                 (update m id update-fn value)
+                                 (update m id update-fn))
+                               ((:assoc-fn spec assoc) m id value))]
+                      (if (:post-validation spec)
+                        (let [[value error] (validate (get m' id) spec opt optarg)]
+                          (if (= value ::error)
+                            [m ids (conj errors error)]
+                            [m' (conj ids id) errors]))
+                        [m' (conj ids id) errors])))
                   [m ids (conj errors error)]))
               [m ids (conj errors (str "Unknown option: " (pr-str opt)))]))
           [defaults [] []] tokens)

src/test/clojure/clojure/tools/cli_test.cljc

@@ -127,14 +127,18 @@
                     :validate [#(not= \/ (first %)) "Must be a relative path"
                                ;; N.B. This is a poor way to prevent path traversal
                                #(not (re-find #"\.\." %)) "No path traversal allowed"]]
+                   ["-l" "--level"
+                    :default 0 :update-fn inc
+                    :post-validation true
+                    :validate [#(<= % 2) #(str "Level " % " is more than 2")]]
                    ["-q" "--quiet"
                     :id :verbose
                     :default true
                     :parse-fn not]])]
       (is (= (parse-option-tokens specs [[:long-opt "--port" "80"] [:short-opt "-q"] [:short-opt "-f" "FILE"]])
-             [{:port (int 80) :verbose false :file "FILE"} []]))
+             [{:port (int 80) :verbose false :file "FILE" :level 0} []]))
       (is (= (parse-option-tokens specs [[:short-opt "-f" "-p"]])
-             [{:file "-p" :verbose true} []]))
+             [{:file "-p" :verbose true :level 0} []]))
       (is (has-error? #"Unknown option"
                       (peek (parse-option-tokens specs [[:long-opt "--unrecognized"]]))))
       (is (has-error? #"Missing required"
@@ -145,6 +149,9 @@
                       (peek (parse-option-tokens specs []))))
       (is (has-error? #"0 is not between"
                       (peek (parse-option-tokens specs [[:long-opt "--port" "0"]]))))
+      (is (has-error? #"Level 3 is more than 2"
+                      (peek (parse-option-tokens specs [[:short-opt "-f" "FILE"]
+                                                        [:short-opt "-l"] [:short-opt "-l"] [:long-opt "--level"]]))))
       (is (has-error? #"Error while parsing"
                       (peek (parse-option-tokens specs [[:long-opt "--port" "FOO"]]))))
       (is (has-error? #"Must be a relative path"