babashka / nbb

Scripting in Clojure on Node.js using SCI
Eclipse Public License 1.0
863 stars 52 forks source link

keyword arg functions do not accept maps #308

Closed ikappaki closed 1 year ago

ikappaki commented 1 year ago

version

v1.2.163

platform

any

problem

keyword argument functions do not accept maps, as per new feature in Clojure 1.11 @ https://clojure.org/news/2021/03/18/apis-serving-people-and-programs.

it throws an errorinstead:

Error: No value supplied for key: {:a 1, :b 2}

repro

  1. open up an nbb repl
  2. Create a keyword argument function as per clojure example and call it with a map as the first argument
    
    (defn destr [& {:keys [a b] :as opts}]
    [a b opts])

(destr :a 1) ;; ->[1 nil {:a 1}]

(destr {:a 1 :b 2}) ;;-> #error {:message "No value supplied for key: {:a 1, :b 2}", :data {:type :sci/error, :line 1, :column 1, :message "No value supplied for key: {:a 1, :b 2}", :sci.impl/callstack #object[cljs.core.Volatile {:val ({:line 1, :column 1, :ns #object[Hr user], :file nil, :sci.impl/f-meta {:name destr, :ns #object[Hr user], :file nil, :arglists ([& {:keys [a b], :as opts}]), :line 1, :column 1}} {:line 1, :column 1, :ns #object[Hr user], :file nil, :sci.impl/f-meta {:name destr, :ns #object[Hr user], :file nil, :arglists ([& {:keys [a b], :as opts}]), :line 1, :column 1}})}], :file nil}, :cause #object[Error Error: No value supplied for key: {:a 1, :b 2}]}



**expected behavior**

The map arg should work and the `(destr {:a 1 :b 2})` should return `[1 2 {:a 1, :b 2}]` instead.

The same work on `babashka` so this doesn't appear to be a `sci` issue?

Thanks
borkdude commented 1 year ago

Duplicate of #201. I would try to reduce the usage of [& {:keys ...}] though. I think the ability to pass maps to legacy [& { }] functions was added not to introduce more of them.

ikappaki commented 1 year ago

Duplicate of #201. I would try to reduce the usage of [& {:keys ...}] though. I think the ability to pass maps to legacy [& { }] functions was added not to introduce more of them.

My use case is this; i have a def map with default values that I would like to pass on to the function, but would also like to call this fn from the repl with keyword arguments. I think both are legitimate use cases, and i personally wouldn't like to choose one over the other.

The distinction they made at the news page is about APIs between people and programs, it doesn't seem to suggest the old expr is bad.

Looking at #201 that has been open for some time now, I take it there is some hesitation enabling this on nbb?

Thanks,

borkdude commented 1 year ago

I agree that this should supported, just because Clojure supports it. The work just needs to be done in SCI.

ikappaki commented 1 year ago

I agree that this should supported, just because Clojure supports it. The work just needs to be done in SCI.

ah ok, I thought for a moment that the implementation in SCI was the same for clj and cljs :)

borkdude commented 1 year ago

It is, but the CLJs implementation is behind since the support for the named arguments landed only later in ClojureScript. Also it might be a challenge to support older CLJS versions, but we'll see. Please upvote the other issue.

borkdude commented 1 year ago

Fixed in 1.2.164

ikappaki commented 1 year ago

Fixed in 1.2.164

Thanks, I can confirm it works now.