Consider having `--no-foo` trigger a validation error for non boolean options

[lread]

Currently

It's nice that bb cli supports --no-foo:

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

(cli/parse-opts ["--no-foo"])
;; => {:foo false}

(cli/parse-opts ["--no-foo"] {:spec {:foo {:coerce :boolean}}})
;; => {:foo false}

But what if foo is not a boolean?

When false is coercible, it doesn't fail...

(cli/parse-opts ["--no-foo"] {:spec {:foo {:coerce :symbol}}})
;; => {:foo false}

(cli/parse-opts ["--no-foo"] {:spec {:foo {:coerce :string}}})
;; => {:foo "false"}

(cli/parse-opts ["--no-foo"] {:spec {:foo {:coerce :keyword}}})
;; => {:foo :false}

But since this makes no sense, I'm thinking specifying --no-foo here merits an error.

We do get an error when false is not coercible...

(cli/parse-opts ["--no-foo"] {:spec {:foo {:coerce :long}}})
;; => Execution error (ExceptionInfo) at babashka.cli/->error-fn$fn (cli.cljc:263).
;;    Invalid value for option --no-foo: cannot transform input "false" to long

But the error message is not the user-friendliest.

Proposal

Generate a helpful error message when --no- is applied to a non-boolean option.

For the :cause we could create a new :not-negatable or reuse the existing :coerce but with a better message: Cannot negate --foo.

Out of scope

I think the :negatable option is still fine as it is.
It only affects generated help.
If a developer slaps this on a non-boolean option, that's a developer error.

[borkdude]

Yes, --no is only applicable to boolean!

[borkdude]

A third possibility would be to just report it as an unknown option (since --no only applies to boolean options)

[lread]

Hmmm... I like your 3rd option best.

[lread]

Hmmm... bb cli currently only throws Unknown option (:cause :restrict) errors with :restrict true.

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

(cli/parse-opts ["--foo"] {:restrict true})
;; => Execution error (ExceptionInfo) at babashka.cli/->error-fn$fn (cli.cljc:263).
;;    Unknown option: --foo

So, it probably makes more sense for --no-foo on a non-boolean to be a :coerce error.

[borkdude]

ok

[lread]

BTW, I'm happy to do up a PR for this.

[borkdude]

Oh I thought you would never ask! :)

[lread]

Hiya! The code describes ::implicit-true-keys set as internal:

cli/src/babashka/cli.cljc

Lines 480 to 482 in 4ac39e3

	`:restrict`/`:require`/`:validate`. Result map includes
	`:org.babashka/cli` metadata and internal `::implicit-true-keys` /
	`::keys-order` metadata used by `coerce-opts`.

Are you ok with me changing it to an ::implicit-values map? I'd like to also track implicit false values to support this change.

Similarly, there is an :implicit-true on ex-info data:

cli/src/babashka/cli.cljc

Lines 119 to 124 in 4ac39e3

	(defn- throw-coerce [s implicit-true? f e]
	(throw (ex-info (str "Coerce failure: " (coerce-failure-reason s implicit-true? f))
	(cond-> {:input s
	:coerce-fn f}
	implicit-true? (assoc :implicit-true true))
	e)))

Would you be ok with me changing that to :implicit-value?

Nothing public is using these, so far as I can tell:
https://grep.app/search?q=%3Aimplicit-true
https://grep.app/search?q=implicit-true-keys

That said, I could preserve existing keys...

Soo...

Option 1: replace ::implicit-true-keys with ::implicit-values and :implict-true with :implicit-value

Option 2: Add ::implicit-values and implicit-value but preserve ::implicit-true-keys and :implicit-true

Option 3: Option 2, plus also add ::implicit-false-keys and :implicit-false for symmetry.

I was coding up Option 1 when I noticed some tests looking for these keys:

cli/test/babashka/cli_test.cljc

Lines 1314 to 1329 in 1b0f5d3

	(testing "error data includes :implicit-true for implicit-true coerce failures"
	;; `--foo` with no value parses to (implicit) `true`. If `:foo` has a
	;; coerce that rejects boolean true (e.g. `:string`), error data
	;; should expose `:implicit-true true` so downstream error mappers
	;; can distinguish "user typed --foo alone" from a real coerce failure.
	(let [errors (atom [])]
	(cli/parse-opts ["--foo"] {:coerce {:foo :string}
	:error-fn (fn [e] (swap! errors conj e))})
	(is (= true (:implicit-true (first @errors))))
	(is (= :coerce (:cause (first @errors))))))
	(testing "error data does NOT include :implicit-true for explicit value failures"
	(let [errors (atom [])]
	(cli/parse-opts ["--foo" "abc"] {:coerce {:foo :long}
	:error-fn (fn [e] (swap! errors conj e))})
	(is (nil? (:implicit-true (first @errors))))
	(is (= :coerce (:cause (first @errors))))))

cli/test/babashka/cli_test.cljc

Lines 1387 to 1393 in 1b0f5d3

	(deftest internal-meta-not-leaked-test
	(testing "::implicit-true-keys not in parse-opts result meta"
	(is (nil? (:babashka.cli/implicit-true-keys (meta (cli/parse-opts ["--foo"]))))))
	(testing "::keys-order not in parse-opts result meta"
	(is (nil? (:babashka.cli/keys-order (meta (cli/parse-opts ["--foo" "--bar" "1"]))))))
	(testing "::key->flag not in parse-opts result meta"
	(is (nil? (:babashka.cli/key->flag (meta (cli/parse-opts ["--foo" "--bar" "1"])))))))

cli/test/babashka/cli_test.cljc

Lines 1408 to 1412 in 1b0f5d3

	(testing "parse-opts* exposes ::implicit-true-keys + ::keys-order + ::key->flag in meta"
	(let [r (cli/parse-opts* ["--foo" "--bar" "1"] {})]
	(is (= #{:foo} (:babashka.cli/implicit-true-keys (meta r))))
	(is (= [:foo :bar] (:babashka.cli/keys-order (meta r))))
	(is (= {:foo "--foo" :bar "--bar"} (:babashka.cli/key->flag (meta r))))))

[borkdude]

Yes, internal keys are always fine to change