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

Add merge function #34

Open
samuelludwig opened this issue Mar 7, 2023 · 6 comments
Open

Add merge function #34

samuelludwig opened this issue Mar 7, 2023 · 6 comments

Comments

@samuelludwig
Copy link

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
Copy link
Owner

borkdude commented Mar 7, 2023

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

@borkdude
Copy link
Owner

borkdude commented Mar 7, 2023

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

@samuelludwig
Copy link
Author

samuelludwig commented Mar 9, 2023

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
Copy link
Author

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

@borkdude
Copy link
Owner

I'll take a look tomorrow!

@borkdude
Copy link
Owner

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.

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