akhudek / fast-zip

fast-zip
85 stars 16 forks source link

changed? not propagated properly #7

Open noprompt opened 9 years ago

noprompt commented 9 years ago

Observe

(require '[fast-zip.core :as fz])
(require '[clojure.zip :as z])

(def xml {:content [{:content ["foo"]}]})

(-> (fz/xml-zip xml)
    (fz/append-child {:content ["bar"]})
    (fz/down)
    (.. path changed?))
;; => nil
(-> (z/xml-zip xml)
    (z/append-child {:content ["bar'"]})
    (z/down)
    (second)
    (get-in [:ppath :changed?]))
;; => true

I'm not sure the best way to fix this right away but I'm investigating it.

noprompt commented 9 years ago

It appears the problems are with down and up.

I resolved the problem by changing down to maintain the :changed? state of the path

           (.children loc)
           (.make-node loc)
           (first cs)
-          (ZipperPath. '() (clojure.core/next cs) path (if path (conj (.pnodes path) node) [node]) nil))))))
+          (ZipperPath. '()
+                       (clojure.core/next cs)
+                       path
+                       (if path (conj (.pnodes path) node) [node])
+                       (when path (assoc path :changed? true))))))))

and for up I added the creation of a new ZipperPath in the else clause

     (.children loc)
     (.make-node loc)
     node
-    (if-let [path (.path loc)] (assoc path :changed? true))))
+    (if-let [path (.path loc)]
+      (assoc (.path loc) :changed? true)
+      (ZipperPath. '() nil nil nil true))))

The tests still pass for these changes but I'll need to update the suite to handle these cases. I will submit another pull request after #6 is merged.