Custom coercer and implicit true

[acamargo]

Problem Description

I'm trying to achieve a specific behaviour for a command-line option using babashka.cli:

1. When the option is not provided, it should not be returned in the parsed options.
2. When the option is provided without an argument (e.g., --option), it should return the option with a specific default value.
3. When the option is provided with an argument (e.g., --option some-value), it should return the option with the provided argument.

Attempts to Solve

I initially tried using the :default key in the spec, but bb.cli includes the option with the default value even when the flag isn't present on the command line.

(cli/parse-opts [] {:spec {:diff {:default "origin/HEAD"}}})
=> {:diff "origin/HEAD"}

Next, I explored the implicit true behaviour for options without a declared coercer:

(cli/parse-opts ["--diff"] {:spec {:diff {}}})
=> {:diff true}

(cli/parse-opts ["--diff" "maoe"] {:spec {:diff {}}})
=> {:diff "maoe"}

This seems promising for points 2 and 3. I then attempted to use a :coercer function to transform the implicit true into my desired default value ("origin/HEAD"):

(cli/parse-opts ["--diff"] {:spec {:diff {:coerce #(if (= "true" %) "origin/HEAD" %)}}})
; Execution error (ExceptionInfo) at babashka.cli/parse-opts$fn (cli.cljc:328).
; Coerce failure: cannot transform (implicit) true with clojure_lsp.main$eval57517$fn__57518@68d3df6c

This resulted in an error. After reviewing the source code, I understood that when an option is treated as "implicit true" (because no argument was provided and no coercer type like :string was specified), the coercer function is specifically checked to ensure it returns true. If it returns anything else, a "Coerce failure" exception is thrown. The relevant check appears to be around https://github.com/babashka/cli/blob/6bbab1fbaa5cacf27b0e516bb50f481d2b05d546/src/babashka/cli.cljc#L139

Proposed Change

I propose modifying the behaviour for implicit true options when a function is provided as the :coercer. Instead of strictly requiring the coercer function to return true, the function should be executed with the implicit true value as input, and its return value should be accepted. Validation via :validate would still apply afterwards.

This change would allow users to provide custom logic within the coercer function to handle the implicit true case, such as substituting a default value or performing a specific transformation.

The modification to the code might look something like this diff:

diff --git a/src/babashka/cli.cljc b/src/babashka/cli.cljc
index 4077abf..a216267 100644
--- a/src/babashka/cli.cljc
+++ b/src/babashka/cli.cljc
@@ -136,7 +136,7 @@
                    (catch #?(:clj Exception :cljs :default) e
                      (throw-coerce s implicit-true? f e)))
               s)]
-    (if (and implicit-true? (not (true? res)))
+    (if (and (not (fn? f)) implicit-true? (not (true? res)))
       (throw-coerce s implicit-true? f nil)
       res)))
 
diff --git a/test/babashka/cli_test.cljc b/test/babashka/cli_test.cljc
index aeedabc..295fe72 100644
--- a/test/babashka/cli_test.cljc
+++ b/test/babashka/cli_test.cljc
@@ -93,7 +93,10 @@
          (cli/parse-opts [":foo"] {:coerce {:foo :string}})))
     (is (thrown-with-msg?
          Exception #"cannot transform \(implicit\) true"
-         (cli/parse-opts [":foo"] {:coerce {:foo [:string]}}))))
+         (cli/parse-opts [":foo"] {:coerce {:foo [:string]}})))
+    (testing "with custom coerce fn"
+      (is (submap? '{:diff "origin/HEAD"}
+                   (cli/parse-opts ["--diff"] {:coerce {:diff (fn [_] "origin/HEAD")}})))))
   (testing "composite opts"
     (is (= {:a true, :b true, :c true, :foo true}
            (cli/parse-opts ["--foo" "-abc"]))))

[borkdude]

@acamargo Why not do something like this in user space? (if (true? diff) "origin/HEAD" diff)

[acamargo]

Keep options logic in the spec only. But perhaps that's just being overly precious.

[borkdude]

@acamargo OK, PR welcome and let's see if anything breaks 😅

[acamargo]

More context, I'm evaluating replacing tools.cli with bb.cli in clojure-lsp because of this.

[borkdude]

PR welcome