Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

missing collection opt value is parsed to contain true #58

Closed
trptr opened this issue Feb 18, 2023 · 1 comment
Closed

missing collection opt value is parsed to contain true #58

trptr opened this issue Feb 18, 2023 · 1 comment

Comments

@trptr
Copy link

trptr commented Feb 18, 2023

(cli/parse-opts [":opt"] {:coerce {:opt [:string]}})

current:

{:opt ["true"]}

expected:

{:opt []}

Notes

1.
The given example should not result {:opt [""]}.

If [""] is wanted, then it is possible via:

(cli/parse-opts [":opt" ""] {:coerce {:opt [:string]}})
{:opt [""]}

2.

(cli/parse-opts [":opt"] {:coerce {:opt [:boolean]}})

current:

{:opt [true]}

expected:

{:opt []} ;; inconsistent to the non-collection version, where missing value defaults to true?

or

{:opt [true]} ;; inconsistent to the proposal for other collections, where missing value results empty collection

I don't have a strong opinion here, but I would vote for {:opt []}, keeping "empty means true" an exception exclusively limited to (non collection) booleans.

3.
The other common collections (#{}, ()) should work similarly & data type (:string, :keyword, :symbol, :int, ...) should not matter:

(cli/parse-opts [":opt"] {:coerce {:opt (list :keyword)}})
{:opt ()}
(cli/parse-opts [":opt"] {:coerce {:opt #{:int}}}) ;; currently throws
{:opt #{}}

4.

(cli/parse-opts [":opt" "a" ":opt"] {:coerce {:opt [:string]}})
{:opt ["a"]} ;; so we can't distinguish  [":opt" "a" ":opt"] and  [":opt" "a"]...

;; alternatively we could throw in this case,
;; because no acceptable value is given for one :opt (the last one in this example) 

5.
The proposed functionality can be achieved by changing add-val to the following. (This is, I guess, not the most performant alternative...)

(defn- add-val [acc current-opt collect-fn coerce-fn arg implicit-true?]
  (let [arg (delay (if (and coerce-fn
                            (not (coll? coerce-fn))) (coerce* arg coerce-fn implicit-true?)
                       (auto-coerce arg)))]
    (if collect-fn
      (if implicit-true?
        (update acc current-opt collect-fn)
        (update acc current-opt collect-fn @arg))
      (assoc acc current-opt @arg))))
@borkdude
Copy link
Contributor

borkdude commented Feb 19, 2023

The fix for #58 was the same as the fix for #57 since --opt without an additional value is always interpreted as "implicit true", i.e. a boolean flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants