Skip to content

Missed some changes to ClojureScript#14

Open
ILoveHubGit wants to merge 16 commits intodaviddpark:masterfrom
ILoveHubGit:master
Open

Missed some changes to ClojureScript#14
ILoveHubGit wants to merge 16 commits intodaviddpark:masterfrom
ILoveHubGit:master

Conversation

@ILoveHubGit
Copy link
Copy Markdown
Contributor

Sorry but with my previous pull request I broke your library. Here an update.
I think that I was somewhat to eager in solving the conflicts.

Hope I did a better job this time.

clj-json-patch.core=> (use 'midje.repl)
Run `(doc midje)` for Midje usage.
Run `(doc midje-repl)` for descriptions of Midje repl functions.
nil
clj-json-patch.core=> (autotest)

======================================================================
Loading (clj-json-patch.util clj-json-patch.rfc6901-test clj-json-patch.core clj-json-patch.util-test clj-json-patch.core-test)
nil
All checks (103) succeeded.
[Completed at 23:34:33]
true

I now ran the tests within a REPL, because on my PC lein test does not seem to work as no test is run then.

@daviddpark
Copy link
Copy Markdown
Owner

daviddpark commented Mar 19, 2020

Thanks for the library updates!

Interesting, because the unit tests passed when I ran them from your code.

Run lein midje. I guess I never documented that in the README.md.

Taking a look now.

@ILoveHubGit
Copy link
Copy Markdown
Contributor Author

Correct. For me the unit tests worked also.
But lein midje only checks the Clojure implementation and not the ClojureScript implementation.
I ran into this error when I tried to use the new version 0.1.7

I'm working on another update so it is possible to run all tests also against ClojureScript.
So you can expect another pull request in the near future :-)

@ILoveHubGit
Copy link
Copy Markdown
Contributor Author

Sorry it took some extra time to solve the problems I ran into. I added a test in which a vector contains multiple maps of which two are changed. This was the JSON part that went wrong in my previous tests. To solve this I changed the order of the condition in the function diff-vecs.
I didn't add the promised ClojureScript automatic tests. But I tested them all in a test application I wrote (https://github.com/ILoveHubGit/testjsonpatch)

Thanks for your patience.

@ILoveHubGit
Copy link
Copy Markdown
Contributor Author

At last I succeeded in adding also the unit tests for ClojureScript. I only implemented the tests that were in core_test.clj because the function tested in the other functions are already tested via Clojure (lein midje).

I would love to see this merged with your library.

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

Successfully merging this pull request may close these issues.

2 participants