Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Oct 30, 2018

image

https://github.com/lambdaisland/deep-diff

Quick and dirty, sorry. Consider this more like an issue than a PR. Feel free to close.

@arichiardi
Copy link
Collaborator

Wow that's very cool!

Me like it 😄 is it Cljs compatible as well?

@plexus
Copy link

plexus commented Oct 30, 2018

@arichiardi no, it uses clj-diff for diffing sequences, and that one seems to be JVM only. Not sure about the other dependencies. I guess Puget and Fipp are cross platform. Not sure about arrangement.

This has been extracted from Kaocha which is JVM only, so it wasn't really an issue there. Would be cool to port it though.

@plexus
Copy link

plexus commented Nov 4, 2018

Thanks for checking out the upstream dependencies. I added an issue on deep-diff: lambdaisland/deep-diff2#2 for people that are also wondering about CLJS support.

Is there any reason you might want to adopt editscript beyond cljs support ?

I wasn't aware of editscript before writing deep-diff. It might or might not be a better diffing backend than cj-diff, but switching to it would mean a complete rewrite of lambdaisland.deep-diff.diff, which I don't think is the best use of my time right now. I added it to the README so I could find it again if I ever want to look at it, but I don't plan to do much with it right now.

@pjstadig
Copy link
Owner

pjstadig commented Nov 5, 2018

I don't want to rely on coloring to tell what has been added and what has been removed. Aside from coloring, '+' and '-' added to the front of numbers can be ambiguous as to whether the number is being added or removed or just happens to be a negative number. The '+' added to the front of :c indicates that the entire pair has been added? I find it a bit difficult to decipher, but maybe I'm just not used to it?

Not that the existing output couldn't be improved in some way, but unless it turns out that most people would prefer this, I'm inclined to keep it the same. Sorry!

@pjstadig pjstadig closed this Nov 5, 2018
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.

3 participants