weavejester / medley

A lightweight library of useful Clojure functions
Eclipse Public License 1.0
870 stars 67 forks source link

Add update-existing-in #38

Closed eraserhd closed 5 years ago

eraserhd commented 5 years ago

Like update-in, except do not update missing keys.

weavejester commented 5 years ago

Thanks for the PR. It's a good change, but I think we should go with something closer to the definition of clojure.core/update-in, so:

(defn update-existing-in [m ks f & args]
  (let [up (fn up [m ks f args]
             (let [[k & ks] ks]
               (if-let [kv (find m k)]
                 (if ks
                   (assoc m k (up (val kv) ks f args))
                   (assoc m k (apply f (val kv) args)))
                 m)))]
     (up m ks f args)))
eraserhd commented 5 years ago

@weavejester commit amended and force-pushed. Your code is unchanged, except for docs and metadata.

I was wondering this while looking at clojure.core/update-in's source, but I guess Clojure is smart enough to not try to not make a thunk when there are no variables closed over, and that's why f and args are passed to up?

weavejester commented 5 years ago

There doesn't seem to be much of a measurable difference in performance between passing f and args as arguments, and passing them via the lexical scope.

I think I like the look of passing them as arguments slightly better, and when in doubt I'm happy to defer to the core implementation, as at least then we can be sure we're not making things any slower.

eraserhd commented 5 years ago

So, I think this is ready then?

weavejester commented 5 years ago

Yep, looks fine. Thanks for your work on this.