replacing & args with options maps #82
Comments
|
Could we get a list of functions here so we don't miss one when we do the work? |
|
I'll look and create a list 👍 |
|
Hi. As a first step, below are all the cases of varargs in functions and macros in the official API namespaces (those appearing under the I will review them to determine which ones are relevant here (cases of options maps as varargs). In the automated analysis below, I omitted a few namespaces because I could not (require '[tech.v3.dataset :as tmd]
'[tech.v3.dataset.print :as print])
(def relevant-namespace-symbols
(->> "deps.edn"
slurp
read-string
:aliases
:codox
:exec-args
:namespaces
(filter (complement #{'tech.v3.datatype.jna
'tech.v3.datatype.char-input
'tech.v3.libs.neanderthal}))
sort))
(apply require relevant-namespace-symbols)
(-> relevant-namespace-symbols
(->> (mapcat (fn [namespace-symbol]
(->> namespace-symbol
the-ns
ns-publics
vals
(filter ifn?)
(sort-by str)
(mapcat (fn [f]
(let [m (meta f)]
(->> m
:arglists
(map (fn [arglist]
{:ns namespace-symbol
:name (:name m)
:arglist arglist}))))))
(filter (fn [{:keys [arglist]}]
(->> arglist
(some #(= % '&)))))))))
tmd/->dataset
(print/print-range :all))_unnamed [59 3]:
|
|
Reviewing the cases above, here are the ones where varargs are used to pass optional arguments of some kind:
|
cnuernber
added
10.000
|
Filtering this further to exclude functions that do in fact take option maps as an optional argument but are destructuring it (which is a performance issue but not what we are fixing here) in the declaration.
I noticed also that there are namespaces that should be removed as they didn't provide useful.
My strategy is going to make the project as correct as possible and either not add deprecated namespaces or do so only when someone really complains as they are actually difficult to do correctly and increase the code size of the finished library. |
|
Fixed in changelist # |
|
Sorry for posting on an already closed issue, but I just saw the recent commit for this and was wondering whether a breaking change is necessary here. Since Clojure 1.11, functions with keyword varargs accept a single options map in addition to the non-wrapped notation, the current dtype API should therefore already support both invokation patterns for most functions: (require '[tech.v3.tensor :as dtt])
(dtt/new-tensor [10 10] :datatype :int8) ; Works
(dtt/new-tensor [10 10] {:datatype :int8}) ; Also already works (using Clojure >= 1.11)By using |
|
This is a very good point - I think worth it to try first. |
Following the recent Zulip conversation about the tensor api, this issue is proposing to generally prefer
optionsmaps over a variable number of arguments (& args).Except for making things more consistent, having a single argument may arguably make things more composable (when passing around options maps, etc.).
This applies to a few functions at the
tech.v3.datatype.functionalandtech.v3.tensornamespaces, and probably a few others.@harold said:
The text was updated successfully, but these errors were encountered: