search

juji-io / editscript

A library to diff and patch Clojure/ClojureScript data structures
Eclipse Public License 1.0
472 stars 23 forks source link

throws when patch and equality test issue #9

Closed huahaiy closed 4 years ago

huahaiy commented 5 years ago

Hi, I finally got around testing this. Unfortunately, I'm running into some issues with this release]. When doing a trivial patch it throws.

(def edit-source {})
(def edit-dest {:x :hello-world})

"this throws"
(let [edits (editscript.edit/get-edits
              (editscript/diff edit-source edit-dest))]
  (editscript/patch edit-source (editscript.edit/edits->script edits))
  )

Throws with:

Execution error (AssertionError) at editscript.edit/edits->script (edit.cljc:198).
Assert failed: Not a vector of valid edits
(valid-edits? edits)

Also, I'm seeing some weird behavior with regard to equality:

(assert (= (editscript.edit/get-edits
                        (editscript/diff {} {}))
          (editscript.edit/get-edits
                        (editscript/diff {} {})))
  "this succeeds")

(def some-edits (editscript.edit/get-edits
                  (editscript/diff {} {})))

(assert (= some-edits (editscript.edit/get-edits
                        (editscript/diff {} {})))
  "this fails")

I created a full repro repo here: https://github.com/FreekPaans/editscript-repro/blob/master/src/editscript_test/core.clj. Also tested on Clojure 1.10.0, Java version was 11.

Originally posted by @FreekPaans in https://github.com/juji-io/editscript/issues/8#issuecomment-532104185

huahaiy commented 5 years ago

@FreekPaans Thank you for the report. It is because the condition of valid-edits? was too strict and some valid edits were considered invalid. I have released a new version 0.4.1 to fix this.

I was unable to reproduce the weird equality behavior though.

FreekPaans commented 5 years ago

Yes, it was because of valid-edits?. I’ll try to have a look at the new version and see if I can look in to the equality stuff a bit more.

On Sat, 21 Sep 2019 at 01:54, Huaha Yang notifications@github.com wrote:

@FreekPaans https://github.com/FreekPaans Thank you for the report. It is because the condition of valid-edits? was too strict and some valid edits were considered invalid. I have released a new version 0.4.1 to fix this.

I was unable to reproduce the weird equality behavior though.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/juji-io/editscript/issues/9?email_source=notifications&email_token=AA35X2LE4QERPDNYWIVK4C3QKVPCJA5CNFSM4IYZ67GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7IFJXI#issuecomment-533746909, or mute the thread https://github.com/notifications/unsubscribe-auth/AA35X2OQBQXTIKGX4HPSZ53QKVPCJANCNFSM4IYZ67GA .

FreekPaans commented 5 years ago

I can confirm that the valid-edits? problem doesn't occur anymore for the examples in the repro. I still need to test it in my actual project with some example data, I'll let you know when I get to that.

WRT equality: how did you test that? When I run it with lein the following happens:


lein run -m   editscript-test.core
Exception in thread "main" java.lang.AssertionError: Assert failed: this fails
(= some-edits (editscript.edit/get-edits (editscript/diff {} {}))), compiling:(editscript_test/core.clj:28:1)
        at clojure.lang.Compiler.load(Compiler.java:7526)
        at clojure.lang.RT.loadResourceScript(RT.java:379)
        at clojure.lang.RT.loadResourceScript(RT.java:370)
        at clojure.lang.RT.load(RT.java:460)
        at clojure.lang.RT.load(RT.java:426)
        at clojure.core$load$fn__6548.invoke(core.clj:6046)
        at clojure.core$load.invokeStatic(core.clj:6045)
        at clojure.core$load.doInvoke(core.clj:6029)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.core$load_one.invokeStatic(core.clj:5848)
        at clojure.core$load_one.invoke(core.clj:5843)
        at clojure.core$load_lib$fn__6493.invoke(core.clj:5888)
        at clojure.core$load_lib.invokeStatic(core.clj:5887)
        at clojure.core$load_lib.doInvoke(core.clj:5868)
        at clojure.lang.RestFn.applyTo(RestFn.java:142)
        at clojure.core$apply.invokeStatic(core.clj:659)
        at clojure.core$load_libs.invokeStatic(core.clj:5925)
        at clojure.core$load_libs.doInvoke(core.clj:5909)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.core$apply.invokeStatic(core.clj:659)
        at clojure.core$require.invokeStatic(core.clj:5947)
        at clojure.core$require.doInvoke(core.clj:5947)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at user$eval149$fn__153.invoke(form-init14102477012176057333.clj:1)
        at user$eval149.invokeStatic(form-init14102477012176057333.clj:1)
        at user$eval149.invoke(form-init14102477012176057333.clj:1)
        at clojure.lang.Compiler.eval(Compiler.java:7062)
        at clojure.lang.Compiler.eval(Compiler.java:7052)
        at clojure.lang.Compiler.load(Compiler.java:7514)
        at clojure.lang.Compiler.loadFile(Compiler.java:7452)
        at clojure.main$load_script.invokeStatic(main.clj:278)
        at clojure.main$init_opt.invokeStatic(main.clj:280)
        at clojure.main$init_opt.invoke(main.clj:280)
        at clojure.main$initialize.invokeStatic(main.clj:311)
        at clojure.main$null_opt.invokeStatic(main.clj:345)
        at clojure.main$null_opt.invoke(main.clj:342)
        at clojure.main$main.invokeStatic(main.clj:424)
        at clojure.main$main.doInvoke(main.clj:387)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.lang.Var.applyTo(Var.java:702)
        at clojure.main.main(main.java:37)
Caused by: java.lang.AssertionError: Assert failed: this fails
(= some-edits (editscript.edit/get-edits (editscript/diff {} {})))
        at editscript_test.core$eval1307.invokeStatic(core.clj:28)
        at editscript_test.core$eval1307.invoke(core.clj:28)
        at clojure.lang.Compiler.eval(Compiler.java:7062)
        at clojure.lang.Compiler.load(Compiler.java:7514)
        ... 40 more```
huahaiy commented 4 years ago

@FreekPaans editscript/diff should be editscript.core/diff

FreekPaans commented 4 years ago

Yeah, it’s aliased in this case (see the require :as)

On Sun, 22 Sep 2019 at 07:14, Huaha Yang notifications@github.com wrote:

editscript/diff should be editscript.core/diff

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/juji-io/editscript/issues/9?email_source=notifications&email_token=AA35X2MG7YAQNOBFUVSWP7DQK35JVA5CNFSM4IYZ67GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7I6UIQ#issuecomment-533850658, or mute the thread https://github.com/notifications/unsubscribe-auth/AA35X2PUPP7LW7EFXOY2WO3QK35JVANCNFSM4IYZ67GA .

huahaiy commented 4 years ago

@FreekPaans I think it is a Clojure bug in version 1.9.0. This equality weirdness only appears on Clojure 1.9.0. The latest Clojure releases, 1.10.x, does not have this problem.

FreekPaans commented 4 years ago

@FreekPaans I think it is a Clojure bug in version 1.9.0. This equality weirdness only appears on Clojure 1.9.0. The latest Clojure releases, 1.10.x, does not have this problem.

Wow, weird. Can confirm it doesn't occur on 1.10.0 and 1.10.1. Any ideas what fixed this in 1.10? Quickly went through the release notes, but didn't see anything that looked like it could be it.

huahaiy commented 4 years ago

My guess would be the fixes in 4.1 Collections. For when the problem occurs in 1.9.0, the diff generated is wrong, [[[] :r {}]] instead of [].

huahaiy commented 4 years ago

Release 0.4.2 to accommodate Clojure 1.9.0.