borkdude / rewrite-edn

Utility lib on top of rewrite-clj with common operations to update EDN while preserving whitespace and comments.
Eclipse Public License 1.0
84 stars 14 forks source link

Add `merge` function #34

Open samuelludwig opened 1 year ago

samuelludwig commented 1 year ago

I think it would be useful to have access to a r/merge function. For example, I've been writing a small side-project that'll produce a new deps/bb.edn file by merging a 'main' file with other supplemental deps.edn files. Having access to a (maybe recursively) merging function would allow you to introduce a whole (nested) map of values in one shot, instead of needing to update/assoc/conj individual keys.

I've tried copying clojure's implementation (which leverages conj) myself using rewrite-edn's r/conj, but I've noticed that r/conj does not interact with maps like clojure.core/conj (expectedly, of course).

I don't think needing to merge groups of nodes is reasonable (I'd think deciding how to maintain formatting would get hairy), so it would be restricted to merging some form/node with a given clojure map.

borkdude commented 1 year ago

Sure. I think we can write merge in terms of the existing assoc, right?

borkdude commented 1 year ago

I think just supporting merging to maps is a good enough start.

samuelludwig commented 1 year ago

Alright, I've been tinkering a little bit and came to this so far, they only work on merging a map into a node of course.

(require '[borkdude.rewrite-edn :as r])

(defn merge* 
  "Merge values from map `m` into node `node`."
  [node m]
  (loop [main-node node
         m-kvs (vec m)]
   (if (empty? m-kvs) 
     main-node
     (let [[k v] (first m-kvs)]
       (recur 
         (r/assoc main-node k v) 
         (rest m-kvs))))))

(defn deep-merge*
  "Recursively merge values from map `b` into node `a`."
  [a b]
  (if (map? (r/sexpr a))
    (merge* a (for [[k v] b] [k (deep-merge* (r/get a k) v)]))
    b))

Given the following file bb.edn

{:paths ["src" "resources"]
 :deps  {borkdude/rewrite-edn {:mvn/version "0.4.6"}} ;; Comment #1
 ;; Comment #2
 :aliases 
  {:neil {:project {:name rw-edn-test/rw-edn-test}}}}
 ;; Comment #3
(def nodes (-> "bb.edn" slurp r/parse-string))

(merge* nodes {:paths ["bb" "scripts"] 
               :deps {'aero/aero {:mvn/version "1.1.16"}} 
               :other [1 :thing]})

;=> 
<forms:
  {:paths ["bb" "scripts"]
   :deps  {aero/aero {:mvn/version "1.1.16"}} ;; Comment #1
   ;; Comment #2
   :aliases 
    {:neil {:project {:name rw-edn-test/rw-edn-test}}}
   :other [1 :thing]}
   ;; Comment #3

>
(deep-merge* nodes {:paths ["bb" "scripts"] 
                    :deps {'aero/aero {:mvn/version "1.1.16"}} 
                    :other [1 :thing]})

;=>
<forms:
  {:paths ["bb" "scripts"]
   :deps  {borkdude/rewrite-edn {:mvn/version "0.4.6"}
           aero/aero {:mvn/version "1.1.16"}} ;; Comment #1
   ;; Comment #2
   :aliases 
    {:neil {:project {:name rw-edn-test/rw-edn-test}}}
   :other [1 :thing]}
   ;; Comment #3

>

This is the functionality I had had in mind, though I feel like my merge implementation is probably clumsy. The deep-merge implementation is a modification of the one found here.

samuelludwig commented 1 year ago

I can try and write up a pull-request tomorrow morning if this looks serviceable.

borkdude commented 1 year ago

I'll take a look tomorrow!

borkdude commented 1 year ago

I think adding the merge you have along with some tests is fine. I want to hold of on deep-merge as deep-merging is often opiniated: should vectors be concatenated or replaced, etc. I think this is best done in user space, but having merge as a primitive in this library makes sense.